-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
managed_hsm
: skip managed hsm auth build if not specified like in cn env
#22400
Conversation
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.
We need to check the Environment not the ResourceIdentifier here?
internal/clients/builder.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("unable to build authorizer for Managed HSM API: %+v", err) | ||
var managedHSMAuth auth.Authorizer | ||
if _, ok := builder.AuthConfig.Environment.ManagedHSM.ResourceIdentifier(); ok { |
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.
the ResourceIdentifier is cloud agnostic - the Environment is cloud-specific, so we'll want to use that:
if _, ok := builder.AuthConfig.Environment.ManagedHSM.ResourceIdentifier(); ok { | |
if _, ok := builder.AuthConfig.Environment.ManagedHSM.Environment(); ok { |
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.
The Api interface has no Environment()
method
terraform-provider-azurerm/vendor/github.com/hashicorp/go-azure-sdk/sdk/environments/interfaces.go
Lines 6 to 12 in 868867d
type Api interface { | |
AppId() (*string, bool) | |
DomainSuffix() (*string, bool) | |
Endpoint() (*string, bool) | |
Name() string | |
ResourceIdentifier() (*string, bool) | |
} |
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.
This should probably be implemented in the SDK, I've opened a PR to add an Available()
method for APIs that we can use in the provider
@wuxu92 We added a new |
@manicminer thank you! @tombuildsstuff updated with the |
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.
LGTM - thanks for this @wuxu92
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
managedHSMAuth actually not used for now. it will be used for managed hsm data plane resources(which has not been merged to main now). managed HSM not suported in china regions (Expected GA in Q4 2023).
fixes: #22374
waiting for #22681 to update sdk.