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

add shared path to SSM parameters #8687

Merged
merged 1 commit into from Nov 30, 2023
Merged

Conversation

konstl000
Copy link
Contributor

Changes proposed by this PR

As of now, it is not possible to configure shared paths for AWS SSM cred manager as opposed to other cred managers such as Vault. Since it is easy to implement and useful, I propose to add a new parameter --aws-ssm-shared-path similar to the Vault cred manager. A path prefix rather than template (as for AWS SecretsManager) makes sense since in this case no default is needed and this parameter can be optional (as with Vault). This PR provides all the changes needed for this functionality.

Notes to reviewer

Release Note

  • Added `--aws-ssm-shared-path` to configure shared secret paths for AWS SSM cred manager similarly to the one for Vault.

go.mod Outdated Show resolved Hide resolved
Signed-off-by: Konstantin Troshin <k.troshin@fme.de>
@konstl000 konstl000 reopened this Jul 28, 2023
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

Gave this a quick look and looks good! Gonna dig into this a bit more when I have more time and then approve if I have no feedback. Thanks for the PR!

@konstl000
Copy link
Contributor Author

Gave this a quick look and looks good! Gonna dig into this a bit more when I have more time and then approve if I have no feedback. Thanks for the PR!

It's been in total already about 9 months for many of the PRs hanging around. There also have been multiple releases during this time. Are there some issues that prevent merging or do you consider if you should merge at all?

@xtremerui
Copy link
Contributor

@taylorsilva ^^

@taylorsilva
Copy link
Member

sorry! Got busy at work ☹️

Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! Sorry for the delay reviewing. Concourse ain't my full time job anymore and my actual job has kept me busy. Thanks for the ping @xtremerui

@taylorsilva taylorsilva merged commit c0523c3 into concourse:master Nov 30, 2023
12 checks passed
@konstl000
Copy link
Contributor Author

No problem, just wanted to know where we are at :-)
Thanks for a quick response and merge!

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

Successfully merging this pull request may close these issues.

None yet

3 participants