Devcenter environments Outputs API Integration #4007
Conversation
wbreza
requested review from
ellismg,
vhvb1989,
hemarina and
weikanglim
as code owners
@wbreza - I know you're going to be on vacation for a few weeks - is there anything you want us to keep track of with respect to the above while you're gone (or a sample project we should be testing with to see when the fix has landed) or do you just want to have this on hold while you are gone (looking at the diff, I suspect the chances of it bitrotting while you are out is small, but I want if there's something we can do while you're gone to keep things moving along, happy to do it). |
I would just keep an heads up on the ADE bi-weekly sync and keep asking about this issue. There isn't any extreme urgency for this to get in before I return. |
cli/azd/pkg/devcenter/manager.go
Outdated
| if requiresJsonMarshalling { | ||
| jsonBytes, err := json.Marshal(devCenterParam.Value) |
There was a problem hiding this comment.
Just checking on this -- what requires us to marshal complex types? It's interesting that we'd allow for int and bool to be set directly, but we'd represent the complex types as JSON-string.
I know in provisioning manager, we'd end up marshaling it for environment outputting, but the Bicep provider doesn't serialize the output either, it just passes the complex types through.
There was a problem hiding this comment.
I believe that ARM deployments API already returns complex types as JSON strings and this is just matching that expectation since these outputs are being set in .env files.
There was a problem hiding this comment.
Indeed the API is returning a JSON string.
When we call json.Unmarshal however, the value would be deserialized as a weakly-typed map[string]interface{}. I'd like to keep a consistent model of what Outputs should represent -- it's the deserialized JSON (weakly-typed). Serializing it here as a JSON string makes it harder to reason about.
Keeping it as map[string]interface{} instead of string also make ADE's provision provider consistent with other providers. Here is what happens today in Bicep provisioning provider when it returns the Outputs to the provision manager, it's a map[string]interface{}:
We only serialize all values to their string-representation when we're saving it to the .env file in provision.Manager:
My best guess is that if we removed JSON marshaling here, the observable-behavior of the system would work just the same, we just made the internal representation more consistent.
There was a problem hiding this comment.
Ok, so you are saying that our UpdateEnvironment func already handles the complexities of marshalling complex types into string... If that is the case then I agree that the Outputs should be consistent with this experience.
…already handled in platform
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
microsoft-github-policy-service
bot
added
the
no-recent-activity
|
Hi @wbreza. Thank you for your interest in helping to improve the Azure Developer CLI experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Hi @wbreza. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
This update moves away from manually reading Azure deployment outputs to leveraging the Outputs API directly from the ADE team. This enables a seamless outputs experience across their supported runners including ARM, Bicep, Terraform and other custom runners.
Important
There is a blocking bug on the ADE side around the serialization of
arrayandobjectoutputs that need to be resolved before we can move forward.