Add configuration to specify where brew is installed

[bombsimon]

Since brew uses different default install locations for Intel and ARM
processors we need to be able to configure where it is installed. This
PR removes all references to the Intel default /usr/local and replaces
that with the new parameter.

Changes

• Add: macOS buildkite_agent_brew_dir to set where brew is installed.

Verification

Installed brew in /opt/homebrew and set buildkite_agent_brew_dir to this path then ran the playbook.

[bombsimon]

Sorry but I can't figure out your workflow. I've run workflow/onboarding.sh and set up everything, and course get the same issues as CI when I run workflow/ci.sh but I don't get how I'm suppose to fix these.

I tried to run yamlfmt -w and apply only the changes to the diff affected by me but to no avail. However, I don't seem to get yamllint to be happy no matter what I do.

Would you mind pointing me in the right direction of how I would go about making CI happy without running yamlfmt on all the files and re-structure and fix all the lint errors?

[Helcaraxan]

Hello @bombsimon. Could you provide some more details of the exact errors that you are encountering. From your description above I am currently not entirely sure of the level at which the errors occur. 🙂

[DoomGerbil]

Would you mind pointing me in the right direction of how I would go about making CI happy without running yamlfmt on all the files and re-structure and fix all the lint errors?

The workflow/ci.sh script, when run locally, should actually make the necessary changes for you where possible (and yamlfmt/yamllint changes should all be automatic). All you should need to do at that point is commit the new changes. I tried checking out your branch and running it, and the script seems to be doing the right thing:

sean@sean-mac-c02xv3a8jgh6 ~/src/ansible-buildkite-agent (develop) $ gh pr checkout 60
remote: Enumerating objects: 8, done.
remote: Counting objects: 100% (8/8), done.
remote: Total 8 (delta 5), reused 8 (delta 5), pack-reused 0
Unpacking objects: 100% (8/8), done.
From github.com:improbable-eng/ansible-buildkite-agent
 * [new ref]         refs/pull/60/head -> brew-path-configurable
Switched to branch 'brew-path-configurable'

sean@sean-mac-c02xv3a8jgh6 ~/src/ansible-buildkite-agent (brew-path-configurable) $ git status
On branch brew-path-configurable
nothing to commit, working tree clean

sean@sean-mac-c02xv3a8jgh6 ~/src/ansible-buildkite-agent (brew-path-configurable) $ workflow/ci.sh
Running pre-commit, incrementally. (You can supply pre-commit flags to this script)...
yamllint.................................................................Failed
- hook id: yamllint
- exit code: 1

<SNIP verbose messages>

Format YAML files........................................................Failed
- hook id: yamlfmt
- files were modified by this hook

defaults/main.yml  Done
handlers/main.yml  Done

Check for added large files..............................................Passed
check BOM - deprecated: use fix-byte-order-marker........................Passed
Check for case conflicts.................................................Passed
Check that executables have shebangs.................(no files to check)Skipped
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check for broken symlinks............................(no files to check)Skipped
Detect Private Key.......................................................Passed
Fix End of Files.........................................................Passed
Forbid new submodules....................................................Passed
Mixed line ending........................................................Passed
Pretty format JSON...................................(no files to check)Skipped
Sort simple YAML files...............................(no files to check)Skipped
Trim Trailing Whitespace.................................................Passed
Forbid binaries......................................(no files to check)Skipped
Test shell scripts with shellcheck...................(no files to check)Skipped
Check shell style with shfmt.........................(no files to check)Skipped
Ansible-lint.............................................................Passed
panic: uncaught error
Traceback (most recent call first):
  at workflow/incremental-pre-commit.sh:6 in main()
pre-commit run --from-ref "origin/develop" --to-ref "HEAD" "${@}" exited 1
panic: uncaught error
Traceback (most recent call first):
  at workflow/ci.sh:5 in main()
workflow/incremental-pre-commit.sh exited 1

sean@sean-mac-c02xv3a8jgh6 ~/src/ansible-buildkite-agent (brew-path-configurable) $ git status
On branch brew-path-configurable
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   defaults/main.yml
	modified:   handlers/main.yml

no changes added to commit (use "git add" and/or "git commit -a")

If you give that a try, does the script make the edits for you?

[bombsimon]

If you give that a try, does the script make the edits for you?

@DoomGerbil Ohh, alright, it does not. I think my environment might not be fully OK then. I had some issues installing the required pyenv version which seems related to macOS 11.1 Big Sur and zlib. Let me troubleshoot that and update the PR.

EDIT: Yeah, this is the result for ci.sh currently:

panic: uncaught error
Traceback (most recent call first):
  at workflow/incremental-pre-commit.sh:6 in main()
pre-commit run --from-ref "origin/develop" --to-ref "HEAD" "${@}" exited 1
panic: uncaught error
Traceback (most recent call first):
  at ./workflow/ci.sh:5 in main()
workflow/incremental-pre-commit.sh exited 1

[DoomGerbil]

If you give that a try, does the script make the edits for you?

Ah, I think I see an issue.

The workflow/ci.sh script automatically makes the yamlfmt changes, but yamllint flags up a bunch of other lint issues (that you did not introduce!) that show up as new because the linter is running in per-file incremental mode, and this file hasn't been modified since before we had pre-commit hooked up.

Thus, the yamllint errors are all new errors about existing lint. And more annoyingly, unlike yamlfmt, yamllint doesn't auto-fix - it's notify-only by design.

[bombsimon]

I got the desired yamlfmt changes to be applied by running pre-commit manually like so:

❯ pre-commit run --from-ref "origin/develop" --to-ref "HEAD" 

I no longer get warnings from yamlfmt but like you said yourself, yamllint still reports a bunch of issues and some are related to my changes, f.ex. line 98 like I mentioned.

I committed and pushed the formatted changes but not sure what to do about linting?

[DoomGerbil]

@bombsimon I've just opened #61 to fix all of these lint issues and adjust our lint config a bit.

We'll get that merged in, and then you can rebase on the updated develop once we do, and you shouldn't need to deal with these issues 😄.

[Helcaraxan]

Regarding the actual content of the PR I would suggest to also define the default values for the new ansible variable as this would otherwise constitute a breaking change. 🙁

[DoomGerbil]

Ok, the lint fixes have been merged - give it a rebase now, and you should have far fewer issues to deal with 😂 .

Thanks!

[bombsimon]

Regarding the actual content of the PR I would suggest to also define the default values for the new ansible variable as this would otherwise constitute a breaking change. 🙁

Hmm, is this true even if I specify it in defaults/main.yaml? Isn't the only way to break this if someone unsets or overwrites buildkite_agent_brew_dir? I don't see any default value for e.g. buildkite_agent_username other than the fact that it's set in the defaults/main.yaml. Or is putting {{ buildkite_agent_brew_dir | default('/usr/local') }} not what you mean?

[DoomGerbil]

Regarding the actual content of the PR I would suggest to also define the default values for the new ansible variable as this would otherwise constitute a breaking change. 🙁

Hmm, is this true even if I specify it in defaults/main.yaml? Isn't the only way to break this if someone unsets or overwrites buildkite_agent_brew_dir? I don't see any default value for e.g. buildkite_agent_username other than the fact that it's set in the defaults/main.yaml. Or is putting {{ buildkite_agent_brew_dir | default('/usr/local') }} not what you mean?

Yeah, I believe that you're right, and this should be OK as is.

Just want to confirm with @Helcaraxan that I'm not missing something else, but this LGTM.

[Helcaraxan]

No. @DoomGerbil is entirely right. This is on me. I had misread the PR and zapped past the point where you set that default. My apologies!

[DoomGerbil]

Cool - approving and merging now. 👍

[DoomGerbil]

@bombsimon You can reference this change at SHA 4bdfaaf to try it out if you like, but I'll cut a 4.1.0 tag today to get this into a proper release version.

[DoomGerbil]

I've just cut https://github.com/improbable-eng/ansible-buildkite-agent/releases/tag/4.1.0 👍

[bombsimon]

Awesome, thank you so much! Really appreciated!