PacoVK / tapir

A Private Terraform Registry
https://pascal.euhus.dev/tapir/
Apache License 2.0
206 stars 15 forks source link

feat: implement scopable deploy keys #445

Closed loispostula closed 2 months ago

loispostula commented 2 months ago

Description

implements #395

Type of change

Please delete options that are not relevant.

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 commented 2 months ago

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

loispostula commented 2 months ago

Hi @PacoVK any chance to get a review soon?

PacoVK commented 2 months ago

@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 commented 2 months ago

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

loispostula commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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!