Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Adding missing Gov regions #1705

Merged
merged 7 commits into from
Nov 8, 2017
Merged

Adding missing Gov regions #1705

merged 7 commits into from
Nov 8, 2017

Conversation

gsacavdm
Copy link
Contributor

@gsacavdm gsacavdm commented Nov 3, 2017

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

pkg/acsengine/azureconst.go is an auto-generated file, so we can't manually update it. It gets its values from one of the scripts under pkg/acsengine/Get-AzureConstants.*

It may be feasible to update those scripts with the programmatic way to add these new government regions. @anhowe might be able to help figure out why we aren't auto-generating usgovarizona and usgovtexas.

@gsacavdm
Copy link
Contributor Author

gsacavdm commented Nov 4, 2017

@jackfrancis , someone is running those commands PS/CLI commands and semi-manually dumping that into the azureconst.go no? I have a hard time imagining this is fully automated because there is no way those commands would return regions from the different sovereign clouds in one single run. Depending on which account you sign in with, these commands will return different regions. So someone needs to be stitching together the results from running these commands across clouds.

If you run these commands yourself, you won't get usgovvirginia nor usgoviowa either, whatever mechanism was used to add those two should be used again.

@jackfrancis
Copy link
Member

Can confirm that the command (for my az account at least) does indeed generate the file you see in pkg/acsengine/azureconst.go:

$ rm -f pkg/acsengine/azureconst.go && python pkg/acsengine/Get-AzureConstants.py
$ git diff pkg/acsengine/azureconst.go 
diff --git a/pkg/acsengine/azureconst.go b/pkg/acsengine/azureconst.go
index 84abfa20..d41fb4ef 100644
--- a/pkg/acsengine/azureconst.go
+++ b/pkg/acsengine/azureconst.go
@@ -1,6 +1,6 @@
 package acsengine
 
-// AUTOGENERATED FILE - last generated 2017-10-26 03:04:32
+// AUTOGENERATED FILE - last generated 2017-11-06 23:13:50
 
 // AzureLocations provides all azure regions in prod.
 // Related powershell to refresh this list:
``

(i.e., the only change to the file is dated comment at the top)

You could read the generator script and investigate if add'l foo would help accommodate gov regional coverage.

@@ -85,6 +85,8 @@ def getFileContents(dcosMasterMap, masterAgentMap, kubernetesAgentMap, sizeMap,
"germanynortheast",
"usgovvirginia",
"usgoviowa",
"usgovarizona",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jackfrancis , turns out the generator scripts have those regions hardcoded, so I just added them there.

@jackfrancis
Copy link
Member

Feel free to run the script and include an updated version of pkg/acsengine/azureconst.go so we can smoke test the script updates. Thanks!

@jackfrancis
Copy link
Member

(I see that pkg/acsengine/azureconst.go doesn't currently have a masthead update, so I assume it was manually updated.)

@gsacavdm
Copy link
Contributor Author

gsacavdm commented Nov 7, 2017

@jackfrancis , done, almost seemed to perfect, came out exactly the same except for the modified header.

@@ -115,6 +115,8 @@ Get-Locations() {
$locationList += "germanynortheast"
$locationList += "usgoviowa"
$locationList += "usgovvirginia"
$locationList += "usgovarizona"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can undo these changes, this file should actually be deleted, since we only use the python file.

anhowe
anhowe previously approved these changes Nov 7, 2017
Copy link
Contributor

@anhowe anhowe left a comment

Choose a reason for hiding this comment

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

lgtm

@jackfrancis
Copy link
Member

@gsacavdm I'll delete the powershell script in a follow-up commit, so no worries there on your account. Kicking off E2E now.

jackfrancis
jackfrancis previously approved these changes Nov 7, 2017
@jackfrancis
Copy link
Member

@gsacavdm could you kindly rebase? We inadvertently moved an unrelated change to azureconst.go and broke this branch :( Will merge forthwith after things are clean, thanks for your patience.

@gsacavdm
Copy link
Contributor Author

gsacavdm commented Nov 8, 2017

There you go @jackfrancis , rebased :)

@jackfrancis jackfrancis merged commit 1416b83 into Azure:master Nov 8, 2017
@jackfrancis
Copy link
Member

Thanks again for the contribution @gsacavdm !

@gsacavdm gsacavdm deleted the patch-1 branch November 8, 2017 05:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants