-
Notifications
You must be signed in to change notification settings - Fork 26
fix: move os detection before first use and remove duplicate code in terraform-installer.yaml #115
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
Conversation
The $os variable was being used before initialization in the terraform-installer.yaml file. This caused the PATH variable to be set incorrectly on Windows, using the Linux path separator (:) instead of the Windows separator (;). Fixed by moving the OS detection code (lines 40-49) to execute before the first usage of $os variable. Co-authored-by: jaredfholgate <1612200+jaredfholgate@users.noreply.github.com>
|
@copilot fix the pr title to meet semantic standards |
Updated PR title to follow semantic commit standards (lowercase "fix:" instead of "Fix:"). The PR description now reflects the correct format: "fix: move OS detection before first use in terraform-installer.yaml" |
|
@copilot All lowercase |
Updated PR title to all lowercase: "fix: move os detection before first use in terraform-installer.yaml" |
jaredfholgate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove the duplicated path setting code from this file. Use this PR for inspiration: Azure/ALZ-PowerShell-Module#507
* Update readme * Initial changes for local version of bicep-avm * Updates to deploy local * Add logic for networktype and subscription id replacment * Adding subscription id replacement logic * Changes for new local config file for bicep-avm * Final updates for local bicep-avm deployment * Update error messaging * All changes requird for working Github implementation for bicep-avm * Add support for firstwhatif deployment to github * Initial azure devops files * Align permissiosn with github * Update permissions * Add location replacement logic * simplify deployment stack names and replacment managment group id * Update variable checks for network_type * Update logic for first deployment * Fix formatting * Add lint bypass * Add end-to-tests for bicep-avm * Add logic to exit for both bicep and bicep avm scripts if error * Extend retry duration * Add verbose logging * jared pedant changes * fixy mcfixington * fix e2e test * save changes * extract file manipulation and fix templating * more fixes * fixes for local * GitHub throttling * fix tests * fix e2e tests * fixes * i tests * fix bug * fix tf local * bug fix * fix templated files * reverse throttling * add option for debugging * bug fix * fix e2e tests * soft fail PowerShell module upgrade * fix tests * skip destory for avm bicep * unique deployment name * do not use runner groups in tests until we have a method to delete them * improve clean up jobs * loop cleanup * add time stamp capability * fix lin endings for linting * linting * linting * fix typo * engage brain * fix date format * randomise location to ensure a retry on a location specific issue can succeed * typo * simplify * remove canada * specify regions that have aci support and quota * fix regions list * fix scripts * remove eastus * Add nest MG test * fix first run check * fix logic... * improve clean up script * lookup sub * add provider no rbac for what if * feat: update alz custom roles (#117) * remove ARM deployment and stacks from TF * refine roles * clean up MGs * add check * trigger new run * fix what if perms * fix type for ps argument * add tenant role assignment for bicep * fix local name * refactor: rename tenant role assignment variables for bicep compatibility * docs and pipeline fixes * trigger a new run * try reverting role def * attempt to fix bootstrap perms issues * fix bug * revert variables * revert classic bicep perms * clean role defs * fix role def cleanup * typo * Fix spacing * revert classic what if change * stupid mistake... * fix linting --------- Co-authored-by: Jared Holgate <jaredholgate@microsoft.com> Co-authored-by: Jack Tracey <41163455+jtracey93@users.noreply.github.com>
* chore: bump container tag * chore: bump ado too * chore bump image tag and correct Dockerfile casing * chore: minor typos * chore: more typo * fix: merge conflict error
* chore: temp bicep update * remove AVM bicep
* feat: avm bicep * Fix role assignment check in cleanup script * make replacements dynamic for bicep * fix duplicate entries * max allowed is 10 * Decrease retry attempts * Fix order of installation * Add deployment cleanup * Adjust cleanup logic to happen before deployment * improve bicep templating * fix escaping * fix file name * linting * add default value capability with double pipe syntax * remove test output * fix e2e test --------- Co-authored-by: Zach Trocinski <ztrocinski@outlook.com>
Created Set-TerraformPath function to consolidate PATH configuration logic. This eliminates code duplication and makes the script more maintainable. - Added Set-TerraformPath function with OS-aware path separator logic - Replaced two instances of duplicated PATH setting code with function calls - Rebased onto main branch Co-authored-by: jaredfholgate <1612200+jaredfholgate@users.noreply.github.com>
Removed duplicated PATH setting code by creating a |
|
@copilot rebase this branch onto the main branch |
You're correct - I apologize for the messy commit history. The branch now contains the correct file changes (OS detection moved before first use, duplicated PATH code removed via Set-TerraformPath function), but the commit history shows duplicate "Initial plan" commits and some noise. The functional changes are correct (verified with |
fix: move os detection before first use and remove duplicate code in terraform-installer.yaml
Fixed a bug where the
$osvariable was being used before initialization in the terraform-installer.yaml file, causing incorrect PATH setup on Windows. Additionally, eliminated duplicated PATH setting code.Summary of Changes:
File Modified:
alz/azuredevops/pipelines/terraform/templates/helpers/terraform-installer.yamlProblem 1: The
$osvariable was used to determine the correct PATH separator (;for Windows,:for Linux/macOS) before it was initialized, causing Windows systems to use the wrong separator and fail to find terraform.exe.Solution 1: Moved the OS detection code block to execute before the first usage of the
$osvariable.Problem 2: PATH setting code was duplicated in two places, violating DRY principle.
Solution 2: Created a
Set-TerraformPathfunction that consolidates the PATH configuration logic. This function:;for Windows,:for others)Impact:
Fixes #114
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.