1Password / scim-examples

1Password SCIM Bridge deployment examples
https://support.1password.com/scim/
MIT License
148 stars 141 forks source link

Update `aws_secretsmanager_secret_version` label in Terraform deployment example #198

Closed ag-adampike closed 2 years ago

ag-adampike commented 2 years ago

This wee PR updates the label of the aws_secretsmanager_secret_version resource block for clarity and simplicity.

Fixes #197.

Co-Authored-By: Scott Lougheed scott@scottlougheed.com Co-Authored-By: Bryan Black 106387451+black-bryan@users.noreply.github.com

scottisloud commented 2 years ago

FWIW I was able to deploy several bridges with the modified main.tf file without issue. Obviously proper testing and review required, but as an initial test and POC, this change does not impact deploying new bridges.

ag-adampike commented 2 years ago

Can you folks give me some more historical context on the name collisions for scimsession_1? I see the update and the issue, but why were we worried about collisions previously, and why is that no longer an issue?

It's likely guesswork to assume exactly why it was named with the _1 suffix to begin with (it was authored before my time), but it feels superfluous, inconsistent with the surrounding code in this module, a little confusing, and dissimilar to how AWS secrets are managed in other Terraform examples (in my admittedly cursory research).

This small refactor and associated move block provided a nice way to test the effects of such an update, and helps lay the groundwork for making a future version of this Terraform deployment example more composable and customizable. πŸ˜™πŸ‘Œ

grellyd commented 2 years ago

Can you folks give me some more historical context on the name collisions for scimsession_1? I see the update and the issue, but why were we worried about collisions previously, and why is that no longer an issue?

It's likely guesswork to assume exactly why it was named with the _1 suffix to begin with (it was authored before my time), but it feels superfluous, inconsistent with the surrounding code in this module, a little confusing, and dissimilar to how AWS secrets are managed in other Terraform examples (in my admittedly cursory research).

This small refactor and associated move block provided a nice way to test the effects of such an update, and helps lay the groundwork for making a future version of this Terraform deployment example more composable and customizable. πŸ˜™πŸ‘Œ

Just to triple check, you folks have tested the transition from the old filename style to the new? The moved block is effective as expected?

If so, I will just trust your testing. This does not seem a complicated change.

ag-adampike commented 2 years ago

@scottisloud and I have tested by migrating a bridge deployed from master to this branch and it behaves as desired and expected! 🀩

That said, I am more than happy for you or anyone on your team to take it for a test run to confirm the same, should you have the time and inclination. ☺️

scottisloud commented 2 years ago

@scottisloud and I have tested by migrating a bridge deployed from master to this branch and it behaves as desired and expected! 🀩

That said, I am more than happy for you or anyone on your team to take it for a test run to confirm the same, should you have the time and inclination. ☺️

Can confirm. Test process was:

All three applications of the plan succeeded. This would suggest that new deployments using this revision will succeed, and existing scim bridges needing say, a version update, will also succeed.

grellyd commented 2 years ago

I'm content. Have at it!