-
Notifications
You must be signed in to change notification settings - Fork 173
[TF-28671] Add HYOK configuration resources #1841
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
base: helenjw/TF-28672/oidc-configurations
Are you sure you want to change the base?
[TF-28671] Add HYOK configuration resources #1841
Conversation
cbe2506
to
944abc1
Compare
resource_tfe_aws_oidc_configuration.go and basic test resource_tfe_gcp_oidc_configuration.go and test resource_tfe_azure_oidc_configuration.go_oidc_configuration.go and test resource_tfe_vault_oidc_configuration.go and tests Add HYOK_ORGANIATION_NAME environment variable to testing.md Add documentation for resources Add to CHANGELOG.md update basic usage in vault_oidc_configuration.html.markdown Do not require replace for everything Update documentation and default value of auth_path skipIfEnterprise update pricing info Implement resource_tfe_hyok_configuration.go Add hyok_configuration.html.markdown fix bug where aws configs recognized as gcp configs Add in acceptance tests Wait for revoked status during acceptance test Add CHANGELOG.md
|
||
AgentPoolID types.String `tfsdk:"agent_pool_id"` | ||
Organization types.String `tfsdk:"organization"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reasoning behind using AWSOIDCConfigurationID
, GCPOIDCConfigurationID
, VaultOIDCConfigurationID
, and AzureOIDCConfigurationID
instead of
oidc_configuration_id
and oidc_configuration_type
like the HyokConfigurations model and database schema uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also a valid way to approach it. At the end of the day, it just needs to populate the tfe.OIDCConfigurationTypeChoice
object that looks like:
type OIDCConfigurationTypeChoice struct {
AWSOIDCConfiguration *AWSOIDCConfiguration
GCPOIDCConfiguration *GCPOIDCConfiguration
AzureOIDCConfiguration *AzureOIDCConfiguration
VaultOIDCConfiguration *VaultOIDCConfiguration
}
I don't really have strong feelings about this, but I think having an explicit type could shorten the code a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this works well, I was just curious if there was a technical reason or constraint. Thanks!
41375cb
to
99097c7
Compare
…8671/hyok-configurations # Conflicts: # CHANGELOG.md # internal/provider/provider_next.go # internal/provider/resource_tfe_aws_oidc_configuration.go # internal/provider/resource_tfe_aws_oidc_configuration_test.go # internal/provider/resource_tfe_azure_oidc_configuration.go # internal/provider/resource_tfe_azure_oidc_configuration_test.go # internal/provider/resource_tfe_gcp_oidc_configuration.go # internal/provider/resource_tfe_gcp_oidc_configuration_test.go # internal/provider/resource_tfe_vault_oidc_configuration.go # internal/provider/resource_tfe_vault_oidc_configuration_test.go
…8671/hyok-configurations
…8671/hyok-configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally, good stuff!
tested for AWS config. I was able to plan, apply and destroy (after revoking the key):
|
gcp test:
|
azure test:
|
vault test:
|
KMSOptions: kmsOptions, | ||
} | ||
|
||
if p.OIDCConfiguration.AWSOIDCConfiguration != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since were doing a switch-case on the top of the file, I'd suggest we do a switch-case here as well?
Description
Describe why you're making this change.
Remember to:
Testing plan
terraform plan
andterraform apply
terraform destroy
on these resources to make sure they can be deletedExternal links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.
Rollback Plan
Changes to Security Controls