Skip to content

Remove canary from default weekly live testing clouds#25589

Merged
benbp merged 1 commit intomainfrom
benbp/weekly-canary-default
Dec 7, 2021
Merged

Remove canary from default weekly live testing clouds#25589
benbp merged 1 commit intomainfrom
benbp/weekly-canary-default

Conversation

@benbp
Copy link
Copy Markdown
Member

@benbp benbp commented Nov 30, 2021

Resolves Azure/azure-sdk-tools#1781

@weshaggard related to the discussion in ^^, I think it adds too much complexity to have a separate "additional clouds" parameter. We already have a bunch of parameters like this where you can pick one to override the default, or the other to append to the default, and it trips people up from time to time. I think in this case given that the set is very static (it will only be changed if we add another sovereign cloud to Azure), the tradeoff is in favor of a single parameter. It also makes it clear at to the tests.yml reader which clouds will actually be tested vs. having to dig into the pipeline template to get the full set.

If this PR goes through on tests and reviews, I'll put ones up for the other repos.

@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label Nov 30, 2021
@benbp benbp self-assigned this Nov 30, 2021
@weshaggard
Copy link
Copy Markdown
Member

So if I understand correctly individual tests.yml need to specify the full list of clouds in order to get the full coverage on the test-weekly runs?

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Nov 30, 2021

So if I understand correctly individual tests.yml need to specify the full list of clouds in order to get the full coverage on the test-weekly runs?

Yes, it was set up this way to enable opt-in to sovereign cloud testing as support is added.

@weshaggard
Copy link
Copy Markdown
Member

Is the name "SupportedClouds" the right one then? I wouldn't have inferred that just by the name. Perhaps providing a snippet here of what the configuration would look like to control the daily vs the weekly would be helpful.

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Dec 3, 2021

Is the name "SupportedClouds" the right one then? I wouldn't have inferred that just by the name. Perhaps providing a snippet here of what the configuration would look like to control the daily vs the weekly would be helpful.

Sorry, I misspoke a little as I was just thinking about the weekly specific flag. For a weekly run it will check inclusion in either Clouds or SupportedClouds.

So to run weekly in ALL clouds, you could do:

Clouds: Public
SupportedClouds: Canary,UsGov,China

or more verbosely:

Clouds: Public
SupportedClouds: Public,Canary,UsGov,China

Or to run weekly for public/usgov/china but exclude canary:

UnsupportedClouds: Canary

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Dec 3, 2021

Currently SupportedClouds defaults to Public,Canary, but I think the original intent was so that we could create all weekly pipelines in one effort, and then opt-in via config, as opposed to creating each new weekly pipeline as we added sovereign cloud support.

@weshaggard
Copy link
Copy Markdown
Member

Ok no matter what we do here we should write the docs on how to configure this as part of the change so we can understand, as currently it is still a little unclear to how to figure out which will run for nightly vs weekly.

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Dec 6, 2021

Ok no matter what we do here we should write the docs on how to configure this as part of the change so we can understand, as currently it is still a little unclear to how to figure out which will run for nightly vs weekly.

I have some documentation on this here and here.

We've chatted before about renaming these variables to something along the lines of Clouds and WeeklyPipelineClouds, but it's another area where it's a tedious amount of work for a low-touch area so I haven't prioritized it highly.

@weshaggard
Copy link
Copy Markdown
Member

It might be worth adding some of those docs in the MD files in the repo instead of in the internal wiki.

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Dec 7, 2021

/check-enforcer override

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Central-EngSys This issue is owned by the Engineering System team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove "Preview" and "Canary" from default weekly live testing clouds

2 participants