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

Metadata fixes #960

Merged
merged 3 commits into from
Apr 11, 2024
Merged

Metadata fixes #960

merged 3 commits into from
Apr 11, 2024

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Apr 9, 2024

Warning

This must be merged in tandem with hashicorp/terraform-provider-azurerm#25546, to avoid breaking storage

  • environments: fix up refreshing from metadata, add missing endpoints/resource IDs for public, usgov and china
  • metadata: remove "support" for metadata 2019-05-01 since it does not include MS Graph and will no longer work with Terraform providers anyway

…include MS Graph and will no longer work with Terraform providers
@manicminer manicminer marked this pull request as ready for review April 10, 2024 19:41
@manicminer manicminer requested a review from a team as a code owner April 10, 2024 19:41
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor things but otherwise LGTM 👍

Comment on lines 41 to 44
env.Attestation = applicationIdOnly("AttestationService", attestationServiceAppId)
env.CDNFrontDoor = applicationIdOnly("CDNFrontDoor", cdnFrontDoorAppId)
env.DataLake = applicationIdOnly("DataLake", dataLakeAppId)
env.IoTCentral = applicationIdOnly("IoTCentral", iotCentralAppId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are applicationIdOnly by default if not configured, so we should be able to remove these?

Suggested change
env.Attestation = applicationIdOnly("AttestationService", attestationServiceAppId)
env.CDNFrontDoor = applicationIdOnly("CDNFrontDoor", cdnFrontDoorAppId)
env.DataLake = applicationIdOnly("DataLake", dataLakeAppId)
env.IoTCentral = applicationIdOnly("IoTCentral", iotCentralAppId)

Comment on lines 40 to 44
// Services not currently available: Attestation, CDNFrontDoor, DataLake, IOTCentral
env.Attestation = applicationIdOnly("AttestationService", attestationServiceAppId)
env.CDNFrontDoor = applicationIdOnly("CDNFrontDoor", cdnFrontDoorAppId)
env.DataLake = applicationIdOnly("DataLake", dataLakeAppId)
env.IoTCentral = applicationIdOnly("IoTCentral", iotCentralAppId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same)

Suggested change
// Services not currently available: Attestation, CDNFrontDoor, DataLake, IOTCentral
env.Attestation = applicationIdOnly("AttestationService", attestationServiceAppId)
env.CDNFrontDoor = applicationIdOnly("CDNFrontDoor", cdnFrontDoorAppId)
env.DataLake = applicationIdOnly("DataLake", dataLakeAppId)
env.IoTCentral = applicationIdOnly("IoTCentral", iotCentralAppId)

Comment on lines 163 to 166
func (e *ApiEndpoint) WithResourceIdentifier(identifier string) Api {
e.resourceIdentifier = pointer.To(identifier)
return e
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's talk of overriding the token below on a per-Storage Account basis - perhaps this should return a new instance of Api rather than mutating the existing object, to ensure this is a clean-copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, will do that 👍

…point` rather than mutating the existing one
@manicminer manicminer merged commit ef82237 into main Apr 11, 2024
5 checks passed
@manicminer manicminer deleted the metadata-fixes branch April 11, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication base-layer bug Something isn't working release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants