Skip to content
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

Retreive agent token from param store on windows agents #762

Merged
merged 2 commits into from
Nov 5, 2020
Merged

Retreive agent token from param store on windows agents #762

merged 2 commits into from
Nov 5, 2020

Conversation

chrisfowles
Copy link
Contributor

fixes #761

Gets agent token from param store if BuildkiteAgentTokenParameterStorePath is set and BuildkiteAgentToken is not for windows instances.

@yob
Copy link
Contributor

yob commented Nov 4, 2020

Thanks @chrisfowles 😍 Missing this on the windows side was definitely an oversight, and confirmed on a test stack based on this PR that the fix works.

My only thought is that current behaviour on linux is: use the SSM path if it's provided, overriding any manually provided token

if [[ -n "${BUILDKITE_AGENT_TOKEN_PATH}" ]] ; then
BUILDKITE_AGENT_TOKEN="$(aws ssm get-parameter --name "${BUILDKITE_AGENT_TOKEN_PATH}" --with-decryption --query Parameter.Value --output text)"
fi

The difference is small, but maybe it's worth using the same behaviour as linux to keep the two code paths as similar as we can?

@chrisfowles
Copy link
Contributor Author

Cheers! I've updated the logic to match the linux install @yob.

There's possibly some discussion around if this is the "correct" logic, as I'd typically expect an explicitly defined value to override a retreived value for application configuration. However it's probably not worth a breaking change on the linux side for just this qualm ;)

@yob yob merged commit 005ebfa into buildkite:master Nov 5, 2020
@yob
Copy link
Contributor

yob commented Nov 5, 2020

lovely, thanks again.

I agree - I'd probably lean towards your initial behavior if we were starting from scratch, but I think the downsides are small enough that consistency with the (only recently) released behaviour is worth it.

I might backport this to a new 5-0-stable branch as well, and if no other significant bugs show up over the next few days then release a 5.0.1 stack.

yob pushed a commit that referenced this pull request Nov 5, 2020
@yob
Copy link
Contributor

yob commented Nov 9, 2020

We've released this in 5.0.1. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows agent doesn't retreive agent token from Parameter Path if specified
2 participants