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

feat: implement scopable deploy keys #445

Merged

Conversation

loispostula
Copy link
Contributor

Description

implements #395

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Improvements

@PacoVK I tested the PR and it works fairly well, the only remaining issues is the migration of existing keys. Is there already a migration mechanism in place? Migrating a key to the scopable one is easy, I just did not find where i could put the logic

@PacoVK
Copy link
Owner

PacoVK commented Sep 9, 2024

@loispostula wow, thank you for this contribution! Good work, i will review it the next days :) Really appreciate your work!

@loispostula
Copy link
Contributor Author

Hi @PacoVK any chance to get a review soon?

@PacoVK
Copy link
Owner

PacoVK commented Sep 17, 2024

@loispostula i am currently on it, but i am also a bit limited in time. So far looks good, to your question, there is not yet a migration mechanism in place. If you have suggestions, they are welcome. Most easy solution is to not migrate the keys, but set the scope per default not to namespace.

@PacoVK
Copy link
Owner

PacoVK commented Sep 18, 2024

@loispostula could you also please add some documentation how eg. a namespace-scoped deploy key should look like?

@loispostula
Copy link
Contributor Author

@PacoVK Regarding the migration, I had a look at implementing a Migrator service, that would work a bit like the bootstrap one, but then figured that I could not construct a scoped key from the existing one. You choose - as as separator to bundle the scope information, but a module name or provider can have - in it's name.

This means that a deploy key called terraform-my_module-tf is easy to migrate but terraform-my-module-tf is not.

Instead of going the migration route, I thus implemented retro-compatibility for existing keys.

Finally regarding documentation, happy to do it, but unsure the best place to do so

@PacoVK
Copy link
Owner

PacoVK commented Sep 22, 2024

@loispostula thanks, yeah makes sense to support backwards-compatibility for the deploykeys.
I took a look and really appreciate your work once again!
Regarding docs you can put some guidance into the usage part. Maybe just about what is the purpose of having module/provider scoped vs. namespace scoped keys. If there are any advantages/disadantages and if you require to follow some naming-pattern (or you think there is a best practice) :)

@loispostula
Copy link
Contributor Author

@PacoVK thanks for the review, I've added the documentation, I think it's plenty sufficient to explain the behavior, but I'm happy to adapt

@PacoVK
Copy link
Owner

PacoVK commented Sep 24, 2024

i think this looks complete! Thanks again, i will try to finish review, integrate and release that soon (hopefully this week already) Thanks again for the good work!

@PacoVK PacoVK merged commit c896e5f into PacoVK:main Sep 26, 2024
@loispostula loispostula deleted the lp/scope-deploy-key branch September 26, 2024 09:41
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.

2 participants