Skip to content

refactor: conditionally use dotfiles in dogfood template #5332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Dec 13, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Dec 6, 2022

Description

When I merged 9e4d213, I had forgotten to ask the user for the dotfiles URI and then use it with coder dotfiles resulting in this log output:

accepts 1 arg(s), received 0          
Run 'coder dotfiles --help' for usage.

Assuming my syntax is correct, this should fix that issue.

Changes

  • feat: add dotfiles_uri var to dogfood template
  • refactor: use dotfiles if dotfiles var exists

This ensures the `coder dotfiles` command only runs if the dotfiles var
in the template is not empty.
dogfood/main.tf Outdated
@@ -34,6 +45,7 @@ resource "coder_agent" "dev" {
code-server --auth none --port 13337 &
sudo service docker start
coder dotfiles -y 2>&1 | tee ~/.personalize.log
"if [ -n ${var.dotfiles_uri} ]; then coder dotfiles var.dotfiles_uri -y 2>&1 | tee ~/.personalize.log; fi"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if this is the exact way to do this interpolation within the Bash script. Smoke-tested locally using terraform validate. cc @bpmct

Ref: Conditionals

Copy link
Member

@johnstcn johnstcn Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to quote the variable in order for -n to work; I'm not sure if terraform will quote it for you.
At least, shellcheck seems to think so.
Also, I believe double brackets are generally preferred for conditionals over single brackets (ref: https://www.baeldung.com/linux/bash-single-vs-double-brackets) Edit: disregard, @mafredri points out that [[ is a bash-ism

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mafredri Shouldn't we have set pipefail in this script?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtojek Since we're supporting /bin/sh, we should avoid -o pipefail (it's a bash-ism).

@jsjoeio jsjoeio changed the title jsjoeio/fix dogfood refactor: conditionally use dotfiles in dogfood template Dec 6, 2022
@jsjoeio jsjoeio requested review from f0ssel and johnstcn December 6, 2022 22:48
@jsjoeio jsjoeio self-assigned this Dec 6, 2022
@jsjoeio jsjoeio marked this pull request as ready for review December 6, 2022 22:50
dogfood/main.tf Outdated
@@ -34,6 +45,7 @@ resource "coder_agent" "dev" {
code-server --auth none --port 13337 &
sudo service docker start
coder dotfiles -y 2>&1 | tee ~/.personalize.log
"if [ -n ${var.dotfiles_uri} ]; then coder dotfiles var.dotfiles_uri -y 2>&1 | tee ~/.personalize.log; fi"
Copy link
Contributor Author

@jsjoeio jsjoeio Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay found this and these are called Heredoc strings

Now I'm second guessing it. Is this what it should be?

Suggested change
"if [ -n ${var.dotfiles_uri} ]; then coder dotfiles var.dotfiles_uri -y 2>&1 | tee ~/.personalize.log; fi"
if [ -n "${var.dotfiles_uri}" ]; then coder dotfiles "${var.dotfiles_uri}" -y 2>&1 | tee ~/.personalize.log; fi

Not sure if the quotes will stay so could be ""${var.dotfilesa-uri}""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@mafredri mafredri Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsjoeio I think a great way to deal with variables is to assign them to a shell variable in the scripts, like this:

DOTFILES_URI=${var.dotfiles_uri}
if [ -n "$DOTFILES_URI" ]; then
   # ...
fi

This way escaping the value becomes a non-issue because the assignment supports basically any input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genius! I'll see if I can wrap this up at the end of the day!

@jsjoeio jsjoeio marked this pull request as draft December 8, 2022 18:44
@jsjoeio jsjoeio marked this pull request as ready for review December 12, 2022 22:50
@jsjoeio jsjoeio requested a review from mafredri December 12, 2022 22:50
@jsjoeio jsjoeio enabled auto-merge (squash) December 13, 2022 21:00
@jsjoeio jsjoeio merged commit 5a568d8 into main Dec 13, 2022
@jsjoeio jsjoeio deleted the jsjoeio/fix-dogfood branch December 13, 2022 21:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants