Azure Spring Apps sub-resources #2045

chutch1122 · 2022-10-24T18:13:55Z

Adds default values to defaultResourcesState.go for Azure Spring Apps sub-resources

Build Service Agent Pool (https://learn.microsoft.com/en-us/rest/api/azurespringapps/build-service-agent-pool)
Config Server (https://learn.microsoft.com/en-us/rest/api/azurespringapps/config-servers)
Monitoring Settings (https://learn.microsoft.com/en-us/rest/api/azurespringapps/monitoring-settings)

github-actions · 2022-10-24T18:15:25Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2022-10-24T18:34:28Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2022-10-24T18:36:18Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2022-10-24T19:29:19Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

chutch1122 · 2022-10-24T19:32:33Z

I think this is ready for review. One thing I did notice is that this repository doesn't seem to have a changelog command in the makefile? (ie: make changelog).

Edit: Got an answer to manually add lines in Slack

jaxxstorm · 2022-10-24T22:50:00Z

/run-acceptance-tests

pulumi-bot · 2022-10-24T22:51:35Z

Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3316791746

github-actions · 2022-10-25T14:09:08Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

danielrbradley · 2022-10-25T16:13:36Z

@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 azure-rest-api-specs submodule: 021e5ab (24th Oct) -> 8d9e220 (20th Oct). I think we want to revert this as this would make us point to an older version. Guessing this might have come in due to rebasing. Can leave this until the last minute as we upgrade versions most nights.

Pending further review:

Confirm source of default values
Review the new resource version set in the v1-config.yaml
Check extreneous changes are removed

chutch1122 · 2022-10-25T16:54:56Z

@danielrbradley

I see the reference links in the description - which part of the docs did you use to infer the default values?

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.

There's been a change to the azure-rest-api-specs submodule: 021e5ab (24th Oct) -> 8d9e220 (20th Oct). I think we want to revert this as this would make us point to an older version. Guessing this might have come in due to rebasing. Can leave this until the last minute as we upgrade versions most nights.

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.

chutch1122 · 2022-10-25T19:07:13Z

@danielrbradley

Here's a gist with the write-up on the default values: https://gist.github.com/chutch1122/0e925605194a62849d62cbbd0570d1ca

github-actions · 2022-10-25T19:59:05Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

danielrbradley · 2022-10-26T12:59:15Z

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.

Add default mappings for resources which can't be deleted. The properties in the map are then used to set values back to in place of doing a delete.
Add a default version for newly available resources to expose them through the SDK.

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:

Should we set the default version of BuildServiceAgentPool to the stable 2022-04-01 version - rather than the preview version as this should have longer-term support.
Should appInsightsInstrumentationKey be being set to nil rather than an empty string ""?
There was no configServer properties shown in your configServers gist - should this be nil rather than an empty object or does this not work in practice?
It doesn't appear there's a 'natural default' for the Build Service Agent Pool size - I see you've selected S1 - is it possible to just not select a default and a 'delete' would just not modify this value?

chutch1122 · 2022-10-26T19:20:06Z

Understanding check

Add default mappings for resources which can't be deleted. The properties in the map are then used to set values back to in place of doing a delete.

Yes.

Add a default version for newly available resources to expose them through the SDK.

Yes.

Questions

Should we set the default version of BuildServiceAgentPool to the stable 2022-04-01 version - rather than the preview version as this should have longer-term support.

I figured since most of the other resources use the 2022-01-01-preview version that I'd keep it consistent. Maybe it would make sense to upgrade everything together to the latest stable version in a follow-up PR?

Should appInsightsInstrumentationKey be being set to nil rather than an empty string ""?

According to the documentation, null or empty string should both work.

There was no configServer properties shown in your configServers gist - should this be nil rather than an empty object or does this not work in practice?

Both an empty object and null both seem to work when using the "Config Servers - Update Put" endpoint. However, using null instead of an empty object produces the same response from the "Config Servers - Get" endpoint as the original configuration so it's probably "more correct".

Basically it comes down to:

GET https://management.azure.com/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.AppPlatform/Spring/{asaServiceName}/configServers/default?api-version=2022-09-01-preview

{
	"properties": {
		"provisioningState": "Succeeded"
                "configServer": {} // This property is returned in the response when you PUT an empty object as configServer. It is omitted when you PUT null as configServer
	},
        // ...
}

It doesn't appear there's a 'natural default' for the Build Service Agent Pool size - I see you've selected S1 - is it possible to just not select a default and a 'delete' would just not modify this value?

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.

github-actions · 2022-10-26T21:29:30Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2022-10-26T21:31:18Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

chutch1122 · 2022-10-26T21:36:04Z

I've reset my branch and updated the config server to be nil

jaxxstorm · 2022-10-27T00:49:13Z

/run-acceptance-tests

pulumi-bot · 2022-10-27T00:50:53Z

Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3333656428

danielrbradley

Happy with all the reasoning and the submodule change now being reverted. This can merge if tests are passing.

danielrbradley · 2022-10-26T12:29:10Z

provider/pkg/openapi/defaultResourcesState.go

+	"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.AppPlatform/Spring/{serviceName}/monitoringSettings/default": {
+		"properties": map[string]interface{}{
+			"traceEnabled":                  false,
+			"appInsightsInstrumentationKey": "",


Is this required in the defaults as the output type seems to be optional - so I assume this would naturally default to null?

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.

…string

github-actions · 2022-10-27T13:46:51Z

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

jaxxstorm · 2022-10-27T14:15:32Z

/run-acceptance-tests

pulumi-bot · 2022-10-27T14:17:10Z

Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3338179770

chutch1122 · 2022-10-27T16:06:42Z

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?

thomas11 · 2022-10-27T17:22:04Z

Looks like there's an issue with the PR build. It says "error: working tree is not clean, aborting!"

@chutch1122, that typically means that something else has changed in the meantime and the SDK needs to be regenerated. ~~I'll take a look.~~ @viveklak has already done it in #2055.

phillipedwards · 2022-10-27T19:27:14Z

/run-acceptance-tests

pulumi-bot · 2022-10-27T19:29:06Z

Please view the PR build: https://github.com/pulumi/pulumi-azure-native/actions/runs/3340312844

viveklak · 2022-10-27T23:04:03Z

@chutch1122 thanks so much for the PR. I pulled this into #2055 to avoid more churn on the PR (needed to run make clean on the PR to remove files for removed types etc.). That was just released in v1.84.0. Thanks very much for your contribution! I will close this PR.

chutch1122 force-pushed the azure-spring-apps branch from f6eef8f to 93fe0fe Compare October 24, 2022 18:33

chutch1122 force-pushed the azure-spring-apps branch from 93fe0fe to fb3255b Compare October 24, 2022 18:34

chutch1122 force-pushed the azure-spring-apps branch from fb3255b to b92a09f Compare October 24, 2022 19:27

mikhailshilkov requested review from danielrbradley and thomas11 October 25, 2022 05:16

Add defaults for Azure Spring Apps sub-resources

d07f6b2

chutch1122 force-pushed the azure-spring-apps branch from 9e1e1c2 to d07f6b2 Compare October 26, 2022 21:28

Update default config server to nil instead of an empty object

4e79751

danielrbradley approved these changes Oct 27, 2022

View reviewed changes

Update default appInsightsInstrumentationKey to nil instead of empty …

012359b

…string

viveklak mentioned this pull request Oct 27, 2022

Azure Spring Apps sub-resources #2055

Merged

viveklak closed this Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure Spring Apps sub-resources #2045

Azure Spring Apps sub-resources #2045

chutch1122 commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

chutch1122 commented Oct 24, 2022 •

edited

jaxxstorm commented Oct 24, 2022

pulumi-bot commented Oct 24, 2022

github-actions bot commented Oct 25, 2022

danielrbradley commented Oct 25, 2022 •

edited

chutch1122 commented Oct 25, 2022 •

edited

chutch1122 commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

danielrbradley commented Oct 26, 2022

chutch1122 commented Oct 26, 2022 •

edited

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

chutch1122 commented Oct 26, 2022

jaxxstorm commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

danielrbradley left a comment

danielrbradley Oct 26, 2022

chutch1122 Oct 27, 2022

github-actions bot commented Oct 27, 2022

jaxxstorm commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

chutch1122 commented Oct 27, 2022

thomas11 commented Oct 27, 2022 •

edited

phillipedwards commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

viveklak commented Oct 27, 2022 •

edited

Azure Spring Apps sub-resources #2045

Azure Spring Apps sub-resources #2045

Conversation

chutch1122 commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

github-actions bot commented Oct 24, 2022

chutch1122 commented Oct 24, 2022 • edited

jaxxstorm commented Oct 24, 2022

pulumi-bot commented Oct 24, 2022

github-actions bot commented Oct 25, 2022

danielrbradley commented Oct 25, 2022 • edited

chutch1122 commented Oct 25, 2022 • edited

chutch1122 commented Oct 25, 2022

github-actions bot commented Oct 25, 2022

danielrbradley commented Oct 26, 2022

chutch1122 commented Oct 26, 2022 • edited

Understanding check

Questions

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

chutch1122 commented Oct 26, 2022

jaxxstorm commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

danielrbradley left a comment

Choose a reason for hiding this comment

danielrbradley Oct 26, 2022

Choose a reason for hiding this comment

chutch1122 Oct 27, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2022

jaxxstorm commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

chutch1122 commented Oct 27, 2022

thomas11 commented Oct 27, 2022 • edited

phillipedwards commented Oct 27, 2022

pulumi-bot commented Oct 27, 2022

viveklak commented Oct 27, 2022 • edited

chutch1122 commented Oct 24, 2022 •

edited

danielrbradley commented Oct 25, 2022 •

edited

chutch1122 commented Oct 25, 2022 •

edited

chutch1122 commented Oct 26, 2022 •

edited

thomas11 commented Oct 27, 2022 •

edited

viveklak commented Oct 27, 2022 •

edited