Conversation
willshu
changed the title
willshu
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying a git branch when cloning dotfiles repositories, enhancing the flexibility of the dotfiles module.
- Introduces two new Terraform variables:
dotfiles_branchanddefault_dotfiles_branch(defaults to "main") - Updates the
coder dotfilescommand to include the--branchflag in the shell script - Adds test coverage for the new branch functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| registry/coder/modules/dotfiles/run.sh | Adds DOTFILES_BRANCH variable and passes it to the coder dotfiles command via --branch flag |
| registry/coder/modules/dotfiles/main.tf | Introduces dotfiles_branch and default_dotfiles_branch variables with corresponding parameter and locals |
| registry/coder/modules/dotfiles/main.test.ts | Adds tests to verify custom branch setting and parameter creation with default value |
| data "coder_parameter" "dotfiles_branch" { | ||
| count = var.dotfiles_branch == null ? 1 : 0 | ||
| type = "string" | ||
| name = "dotfiles_branch" | ||
| display_name = "Dotfiles Branch" | ||
| default = var.default_dotfiles_branch | ||
| description = "The branch to use for the dotfiles repository" | ||
| mutable = true | ||
| icon = "/icon/dotfiles.svg" | ||
| } |
There was a problem hiding this comment.
The dotfiles_branch parameter is missing the order field that's present in the dotfiles_uri parameter (line 83). For consistency and to allow users to control the presentation order of parameters, consider adding order = var.coder_parameter_order to this parameter definition as well.
| fi | ||
|
|
||
| echo "✨ Applying dotfiles for user $DOTFILES_USER" | ||
| echo "✨ Applying dotfiles for user $DOTFILES_USER from branch $DOTFILES_BRANCH" |
There was a problem hiding this comment.
When DOTFILES_BRANCH is an empty string, the echo message will display "from branch " which looks incomplete. Consider adding a conditional check to only display the branch information when it's not empty, or ensure that DOTFILES_BRANCH cannot be empty through validation in the Terraform configuration.
| echo "✨ Applying dotfiles for user $DOTFILES_USER from branch $DOTFILES_BRANCH" | |
| if [ -n "$DOTFILES_BRANCH" ]; then | |
| echo "✨ Applying dotfiles for user $DOTFILES_USER from branch $DOTFILES_BRANCH" | |
| else | |
| echo "✨ Applying dotfiles for user $DOTFILES_USER" | |
| fi |
|
|
||
| if [ "$DOTFILES_USER" = "$USER" ]; then | ||
| coder dotfiles "$DOTFILES_URI" -y 2>&1 | tee ~/.dotfiles.log | ||
| coder dotfiles "$DOTFILES_URI" --branch "$DOTFILES_BRANCH" -y 2>&1 | tee ~/.dotfiles.log |
There was a problem hiding this comment.
The --branch flag is passed even when DOTFILES_BRANCH might be an empty string. If a user explicitly sets dotfiles_branch = "" in Terraform, this will pass --branch "" to the coder CLI, which may cause unexpected behavior. Consider wrapping the command in a conditional to only add the --branch flag when the value is non-empty, or add validation in main.tf to prevent empty string values.
| CODER_BIN=$(which coder) | ||
| DOTFILES_USER_HOME=$(eval echo ~"$DOTFILES_USER") | ||
| sudo -u "$DOTFILES_USER" sh -c "'$CODER_BIN' dotfiles '$DOTFILES_URI' -y 2>&1 | tee '$DOTFILES_USER_HOME'/.dotfiles.log" | ||
| sudo -u "$DOTFILES_USER" sh -c "'$CODER_BIN' dotfiles '$DOTFILES_URI' --branch '$DOTFILES_BRANCH' -y 2>&1 | tee '$DOTFILES_USER_HOME'/.dotfiles.log" |
There was a problem hiding this comment.
The --branch flag is passed even when DOTFILES_BRANCH might be an empty string. If a user explicitly sets dotfiles_branch = "" in Terraform, this will pass --branch '' to the coder CLI, which may cause unexpected behavior. Consider wrapping the command in a conditional to only add the --branch flag when the value is non-empty, or add validation in main.tf to prevent empty string values.
| variable "dotfiles_branch" { | ||
| type = string | ||
| description = "The branch to use for the dotfiles repository (optional, when set, the user isn't prompted for the branch)" | ||
| default = null |
There was a problem hiding this comment.
Consider adding validation to prevent empty string values for dotfiles_branch. An empty branch name could lead to issues when passed to the coder dotfiles CLI command. You can add a validation block like:
validation {
condition = var.dotfiles_branch == null || var.dotfiles_branch != ""
error_message = "dotfiles_branch cannot be an empty string. Use null to prompt the user or provide a valid branch name."
}| default = null | |
| default = null | |
| validation { | |
| condition = var.dotfiles_branch == null || var.dotfiles_branch != "" | |
| error_message = "dotfiles_branch cannot be an empty string. Use null to prompt the user or provide a valid branch name." | |
| } |
|
@willshu Once you have addressed the above issues I will give this a review and test. |
|
@willshu I just wanted to double check and see if there was any update on this |
3 participants
Description
Type of Change
Module Information
Path:
registry/[namespace]/modules/[module-name]New version:
v1.0.0Breaking change: [ ] Yes [ ] No
Template Information
Path:
registry/[namespace]/templates/[template-name]Testing & Validation
bun test)bun fmt)Related Issues