-
Notifications
You must be signed in to change notification settings - Fork 197
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
Support sovereign clouds #2897
Support sovereign clouds #2897
Conversation
40f502d
to
56e1d6d
Compare
56e1d6d
to
74802ed
Compare
Commenter does not have sufficient privileges for PR 2897 in repo Azure/azure-dev |
/azp run azure-dev - cli |
Azure Pipelines successfully started running 1 pipeline(s). |
0bff220
to
6742ca7
Compare
/azp run azure-dev - cli |
Azure Pipelines successfully started running 1 pipeline(s). |
Hey @danieljurek I recommend a couple of quick style tweaks for consistency (see below). Regarding the message itself, is there anyway we could be more specific? When we say "use valid names", what does the user to do? Run the command again? |
The user can set the cloud in one of 3 places:
The command to modify would be different based on the configuration used. We could probably include the used config type which could make this error message more useful. Will need to explore how one can do formatting for the error. |
@Austinauth -- We do have some precedent for formatting the error message and providing suggestions... Following that pattern I get: Note that "Error:" is different from your proposed red "ERROR:" ... If we change formatting for that it changes across I'll work some more on using the config context to provide useful suggestions. |
Here's the current guidance for error message styling. This has been the guidance for the last year or so, but somewhere along the way we've been inconsistent in our application of it. (see: Azure Developer CLI Design Dashboard) We've discussed tweaking the format (casing, color application), but we've never made any formal change to the formatting since defining the above as far as I am aware.
Ahh I see. It's difficult for me to rewrite the suggestion without having a strong technical understanding of what is the root cause of the error and how to resolve it. But basically if the user wants to resolve the issue we need them to go into either their user settings, project settings, or environment and change the "cloud" name to something we accept? Is there an easy way for the user to understand what is a valid cloud name so they can modify theirs? Is there a docs link on the subject? Perhaps we could link to it. |
@Austinauth -- I can produce 3 types of error messages now: |
cli/azd/cmd/cobra_builder.go
Outdated
var suggestionErr *azcli.ErrorWithSuggestion | ||
if errors.As(err, &suggestionErr) { | ||
cmd.SilenceErrors = true | ||
cb.container.Invoke(func(console input.Console) { | ||
console.Message(ctx, color.RedString("ERROR: %s", err.Error())) | ||
console.Message(ctx, (*azcli.ErrorWithSuggestion)(suggestionErr).Suggestion) | ||
}) | ||
return err | ||
} |
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.
@wbreza -- ActionResult
doesn't fit here because we're determining the cloud configuration before executing actions. ErrorWithSuggestion
works and this formats all encountered ErrorWithSuggestion
s accordingly:
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.
@Austinauth right... need to add you to this discussion :)
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.
Just to be clear - at this point we're getting an error when resolving the action because there was some cloud configuration error which internally uses ErrorWithSuggestion
.
This seems correct, but also feel that the handling of this special error scenario should be somewhere else but we have not invoked an action and therefore cannot be handled by a middleware. Feels like maybe this should be in root/main.go when we get the error back from the root level cobra command invocation.
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.
Message and suggestion look great @danieljurek. The suggestion is clear and actionable, and the execution is visually consistent.
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 is a great start but going to stop here for now:
Overall feedback:
- Love the bootstrapping configuration style from the various config options
- Not a fan of having to have
cloud
as a member on so many of our components - Would prefer us to move the injection of the SDK
ClientOptions
and SDK clients into the container. This would allow us to inject those which is already handing thecloud
concerns for the most part.
Goal: Only need to inject cloud
into our components if we need to get an URL prefix or service url.
cli/azd/cmd/cobra_builder.go
Outdated
var suggestionErr *azcli.ErrorWithSuggestion | ||
if errors.As(err, &suggestionErr) { | ||
cmd.SilenceErrors = true | ||
cb.container.Invoke(func(console input.Console) { | ||
console.Message(ctx, color.RedString("ERROR: %s", err.Error())) | ||
console.Message(ctx, (*azcli.ErrorWithSuggestion)(suggestionErr).Suggestion) | ||
}) | ||
return err | ||
} |
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.
Just to be clear - at this point we're getting an error when resolving the action because there was some cloud configuration error which internally uses ErrorWithSuggestion
.
This seems correct, but also feel that the handling of this special error scenario should be somewhere else but we have not invoked an action and therefore cannot be handled by a middleware. Feels like maybe this should be in root/main.go when we get the error back from the root level cobra command invocation.
} | ||
|
||
// Project Configuration | ||
projConfig, _ := lazyProjectConfig.GetValue() |
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.
You likely should check for an error here in the event there is no project / azure.yaml file.
if azdCtx, err := lazyAzdContext.GetValue(); err == nil { | ||
if azdCtx != nil && localEnvStore != nil { | ||
if defaultEnvName, err := azdCtx.GetDefaultEnvironmentName(); err == nil { | ||
env, err := localEnvStore.Get(ctx, defaultEnvName) | ||
if err == nil { | ||
cloudConfigurationNode, exists := env.Config.Get(cloud.ConfigPath) | ||
if exists { | ||
value, err := cloud.ParseCloudConfig(cloudConfigurationNode) | ||
if err == nil { | ||
cloudConfig = value | ||
|
||
// In the event of an error set the suggestion for updating the cloud configuration |
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.
Lots of nesting going on... Any way we can clean this up to short circuit return or similar?
httpClient httputil.HttpClient, | ||
cloud *cloud.Cloud, | ||
) *SubscriptionsService { |
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.
Is there any way we can avoid propagating this up through our exposed service? Maybe there is an opportunity to inject the *armsubscriptions.Client
here instead and leave the internal cloud configuration to the IoC container.
@@ -365,8 +379,10 @@ func (m *Manager) newCredentialFromClientSecret( | |||
options := &azidentity.ClientSecretCredentialOptions{ | |||
ClientOptions: policy.ClientOptions{ | |||
Transport: m.httpClient, | |||
Cloud: m.cloud.Configuration, |
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.
Feels like we could move the policy.ClientOptions
as injectable that automatically handles the cloud configuration vs have to specify this in multiple places.
@@ -61,6 +64,7 @@ func TestServicePrincipalLoginClientSecret(t *testing.T) { | |||
configManager: newMemoryConfigManager(), | |||
userConfigManager: newMemoryUserConfigManager(), | |||
credentialCache: credentialCache, | |||
cloud: publicCloud, |
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.
I'd like to really avoid having to support cloud
as a top level member of our components and move to a model where it is an internal implementation detail of working with the Go SDK.
"description": "Optional. Provides additional configuration for deploying to sovereign clouds such as Azure Government.", | ||
"additionalProperties": false, | ||
"properties": { | ||
"name": { "enum": [ "AzureCloud", "AzureChinaCloud", "AzureUSGovernment" ] } |
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.
Feels like if the user is going to specify the cloud section then the name
should be required.
653fad8
to
b859d12
Compare
… require the user to do that?
091d887
to
936f2c9
Compare
Repoman Generation ResultsRepoman pushed changes to remotes for the following projects: Project: azd-starter-terraformRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/azd-starter-terraform -b pr/2897 View Changes | Compare Changes Project: todo-csharp-cosmos-sqlRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-csharp-cosmos-sql -b pr/2897 View Changes | Compare Changes Project: todo-csharp-sql-swa-funcRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-csharp-sql-swa-func -b pr/2897 View Changes | Compare Changes Project: todo-csharp-sqlRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-csharp-sql -b pr/2897 View Changes | Compare Changes Project: todo-java-mongo-acaRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-java-mongo-aca -b pr/2897 View Changes | Compare Changes Project: todo-java-mongoRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-java-mongo -b pr/2897 View Changes | Compare Changes Project: todo-java-postgresql-terraformRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-java-postgresql-terraform -b pr/2897 View Changes | Compare Changes Project: todo-nodejs-mongo-acaRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-aca -b pr/2897 View Changes | Compare Changes Project: todo-nodejs-mongo-aksRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-aks -b pr/2897 View Changes | Compare Changes Project: todo-nodejs-mongo-swa-funcRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-swa-func -b pr/2897 View Changes | Compare Changes Project: todo-nodejs-mongoRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo -b pr/2897 View Changes | Compare Changes Project: todo-nodejs-mongo-terraformRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-nodejs-mongo-terraform -b pr/2897 View Changes | Compare Changes Project: todo-python-mongo-acaRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-aca -b pr/2897 View Changes | Compare Changes Project: todo-python-mongo-swa-funcRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-swa-func -b pr/2897 View Changes | Compare Changes Project: todo-python-mongoRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo -b pr/2897 View Changes | Compare Changes Project: todo-python-mongo-terraformRemote: azure-samples-stagingBranch: pr/2897You can initialize this project with: azd init -t Azure-Samples/todo-python-mongo-terraform -b pr/2897 View Changes | Compare Changes |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSIContainer
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Superseded by #3452 |
Fixes #973
Some decisions to be aware of:
Only support well-known clouds (Azure Public, China Cloud, US Gov Cloud)
Passing of
pkg/cloud.Cloud
is done as a pointer to follow our normal convention.Configuration panics if a clouds name is unknown so the cloud name must be precise... another approach would be to fall back to public cloud and/or log an error message. I'm adding this in.
SCM hostname for zip deployment is retrieved from ARM to avoid hardcoding additional suffixes
The bulk of configuration business logic is in
cmd/container.go
because putting it inpkg/cloud
would result in an import cycle and, since we're only doing it in one place we should do that here.Some templates needed minor changes to support sovereign clouds
UX change (@Austinauth): If a user sets an incorrect cloud name and attempts to run a command that interacts with the cloud the user will see an error -
Safe default "AzureCloud" is always assumed if
cloud.name
is not set.Docs updates: https://github.com/MicrosoftDocs/azure-dev-docs-pr/pull/5353
Here is a candidate for simplifying injection of client options: https://github.com/Azure/azure-dev/pull/3270/files