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

Chiac/svc #893

Closed
wants to merge 23 commits into from
Closed

Chiac/svc #893

wants to merge 23 commits into from

Conversation

whober0521
Copy link
Contributor

What this PR does

Jira:
Link to demo recording:

Special notes for your reviewer

Copy link

Please rebase pull request.

Comment on lines +411 to +427

// // oidc

// module oidc '../modules/oidc/main.bicep' = {
// name: '${deployment().name}-oidc'
// params: {
// location: location
// storageAccountName: oidcStorageAccountName
// rpMsiName: clusterServiceMIName
// skuName: oidcStorageAccountSku
// aroDevopsMsiId: aroDevopsMsiId
// deploymentScriptLocation: location
// }
// dependsOn: [
// svcCluster
// ]
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

the permission issue has been fixed in main - #897

@@ -21,7 +21,7 @@ func EV2Mapping(input config.Variables, prefix []string) (map[string]string, map
}
replaced[key] = replacement
} else {
placeholder := fmt.Sprintf("__%s__", strings.ToUpper(strings.Join(nestedKey, "_")))
placeholder := fmt.Sprintf("__%s__", strings.Join(nestedKey, "_"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in main including the tests - #901

Comment on lines +374 to +391
// resource imageSyncAcrResourceGroups 'Microsoft.Resources/resourceGroups@2023-07-01' existing = [
// for rg in imageSyncAcrResourceGroupNames: {
// name: rg
// scope: subscription()
// }
// ]

// module acrPushRole '../modules/acr-permissions.bicep' = [
// for (_, i) in imageSyncAcrResourceGroupNames: {
// name: guid(imageSyncAcrResourceGroups[i].id, resourceGroup().name, 'image-sync', 'push')
// scope: imageSyncAcrResourceGroups[i]
// params: {
// principalId: imageSyncManagedIdentityPrincipalId
// grantPushAccess: true
// acrResourceGroupid: imageSyncAcrResourceGroups[i].id
// }
// }
// ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has been removed from main #896

Comment on lines +393 to +410
// resource clustersServiceAcrResourceGroups 'Microsoft.Resources/resourceGroups@2023-07-01' existing = [
// for rg in clustersServiceAcrResourceGroupNames: {
// name: rg
// scope: subscription()
// }
// ]

// module acrManageTokenRole '../modules/acr-permissions.bicep' = [
// for (_, i) in clustersServiceAcrResourceGroupNames: {
// name: guid(clustersServiceAcrResourceGroups[i].id, resourceGroup().name, 'clusters-service', 'manage-tokens')
// scope: clustersServiceAcrResourceGroups[i]
// params: {
// principalId: csManagedIdentityPrincipalId
// grantManageTokenAccess: true
// acrResourceGroupid: clustersServiceAcrResourceGroups[i].id
// }
// }
// ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this code has been moved to cluster-service.bicep and will not run in MSFT INT for the time being until we have the ACR token custom role figured out - #903

@@ -47,7 +47,7 @@ module regionalZoneDelegation '../modules/dns/zone-delegation.bicep' = {
params: {
childZoneName: regionalDNSSubdomain
childZoneNameservers: regionalZone.properties.nameServers
parentZoneName: baseDNSZoneName
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm. this would try to delegate the regional zone into the regional zone as child zone and parent zone would be the same

@geoberle geoberle closed this Dec 6, 2024
@whober0521 whober0521 deleted the chiac/svc branch December 6, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants