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
Azure Spring Apps sub-resources #2045
Conversation
PR is now waiting for a maintainer to run the acceptance tests. |
f6eef8f
to
93fe0fe
Compare
PR is now waiting for a maintainer to run the acceptance tests. |
93fe0fe
to
fb3255b
Compare
PR is now waiting for a maintainer to run the acceptance tests. |
fb3255b
to
b92a09f
Compare
PR is now waiting for a maintainer to run the acceptance tests. |
I think this is ready for review. One thing I did notice is that this repository doesn't seem to have a Edit: Got an answer to manually add lines in Slack |
/run-acceptance-tests |
Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3316791746 |
PR is now waiting for a maintainer to run the acceptance tests. |
@chutch1122 I've started looking through this. Just need to work through a few more of the details to get it in. I see the reference links in the description - which part of the docs did you use to infer the default values? There's been a change to the Pending further review:
|
I created an Azure Spring Apps instance in Azure and pulled the default values by making API requests manually. I'll write this up in a follow-up comment after lunch.
Sounds good. I remember seeing a change there when rebasing. Any advice on how to safely revert that change? I'm not used to working with git submodules. |
Here's a gist with the write-up on the default values: https://gist.github.com/chutch1122/0e925605194a62849d62cbbd0570d1ca |
PR is now waiting for a maintainer to run the acceptance tests. |
Thanks for the gist - that was very helpful 🙇 Here's my summary what I'm currently understanding here, please correct me if I've misunderstood.
I've done a quick rebase for you over here which I think incorporates only the intended changes - feel free to reset your own branch to this commit if the change looks correct to you: https://github.com/pulumi/pulumi-azure-native/compare/azure-spring-apps Pending questions:
|
Understanding check
Yes.
Yes. Questions
I figured since most of the other resources use the
According to the documentation, null or empty string should both work.
Both an empty object and Basically it comes down to:
S1 is the smallest size available for the agent pool so if we can't truly delete it I thought it would be logical to minimize cost. It'd be fine not to modify the value too, of course. I don't have a preference either way. |
9e1e1c2
to
d07f6b2
Compare
PR is now waiting for a maintainer to run the acceptance tests. |
PR is now waiting for a maintainer to run the acceptance tests. |
I've reset my branch and updated the config server to be |
/run-acceptance-tests |
Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3333656428 |
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.
Happy with all the reasoning and the submodule change now being reverted. This can merge if tests are passing.
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.AppPlatform/Spring/{serviceName}/monitoringSettings/default": { | ||
"properties": map[string]interface{}{ | ||
"traceEnabled": false, | ||
"appInsightsInstrumentationKey": "", |
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 this required in the defaults as the output type seems to be optional - so I assume this would naturally default to null?
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.
They both work according to the documentation but null is probably more correct (similar to the config server). I've made a commit that reflects this change.
PR is now waiting for a maintainer to run the acceptance tests. |
/run-acceptance-tests |
Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3338179770 |
Looks like there's an issue with the PR build. It says "error: working tree is not clean, aborting!" What needs to happen to get this fixed? |
@chutch1122, that typically means that something else has changed in the meantime and the SDK needs to be regenerated. |
/run-acceptance-tests |
Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3340312844 |
@chutch1122 thanks so much for the PR. I pulled this into #2055 to avoid more churn on the PR (needed to run |
Adds default values to
defaultResourcesState.go
for Azure Spring Apps sub-resources