Skip to content
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

Add Script for adding Include paths to mgmt yml #9567

Conversation

chidozieononiwu
Copy link
Member

@chidozieononiwu chidozieononiwu commented Jan 21, 2020

No plans to merge this till after Feb 1
cc @erich-wang


Switched to including Management directories.

- master
paths:
exclude:
- sdk/appconfiguration/Azure.Data.AppConfiguration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we chatted given this Pipeline is for mgmt libraries we should do an include for those instead of an exclude for client. That is how all other pipelines in this repo function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the script to include management directories instead.

branches:
include:
- master
- '*-preview'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this branch pattern for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am not sure, was trying to maintain what's currently set on the net - mgmt - ci pipeline. @isra-fel Any ideas why that was added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for public review branches

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those branches in the main repository? How much work would it be to make those branch names conform to the branching naming guidelines at https://github.com/Azure/azure-sdk/blob/master/docs/policies/repobranching.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There are currently 4 such branches, all stale. I believe it's safe and easy to rename them

@chidozieononiwu chidozieononiwu force-pushed the ExcludeMgmtPipelinefromClientRuns branch from b9b2f06 to 304b6a6 Compare January 21, 2020 23:18
@chidozieononiwu chidozieononiwu changed the title Add Script for adding exclusion paths to mgmt yml Add Script for adding Include paths to mgmt yml Jan 21, 2020
- master
paths:
include:
- sdk/advisor/Microsoft.Azure.Management.Advisor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isra-fel do you guys depend on automatic CI runs on every commit or do you just do a nightly or manual runs once changes are in? I ask because we could avoid having this list twice if we only needed it for PR validation and not CI triggering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we only need PR validation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only add includes for the PR validations won't it mean that data-plane changes will still trigger this on CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set CI trigger to none.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the script.

@isra-fel
Copy link
Member

isra-fel commented Feb 5, 2020

Sorry for the late response and thank you for the effort 😀
If I understand this correctly , the idea is to prevent management pipeline from being triggered by client-only pull requests, right?

My only concern is about onboarding new services, PR validation won't be triggered because its path is not in the yaml. I suppose I'll need to manually add it. Is there a better suggestion?

@chidozieononiwu
Copy link
Member Author

Sorry for the late response and thank you for the effort 😀
If I understand this correctly , the idea is to prevent management pipeline from being triggered by client-only pull requests, right?

My only concern is about onboarding new services, PR validation won't be triggered because its path is not in the yaml. I suppose I'll need to manually add it. Is there a better suggestion?

For new services you just need to run the Update-Mgmt-Yml.ps1 script and include the changes to the PR that onboards the new service. I can add instructions for that in the contributing doc.

@isra-fel
Copy link
Member

isra-fel commented Feb 5, 2020

Is it possible to use wildcards in include such as sdk/*/Microsoft.Azure.Management.*?

Copy link
Member

@erich-wang erich-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chidozieononiwu, @weshaggard, how about use exclude instead of include in mgmt.yml? The reasons to use exclude paths:

  • the number of data plane folders are far less than the number of management folders, so it is easier to find which folders is missed
  • if we forget to update this file for new mgmt folder when using include paths, it is real issue; however if we forget to update this file for new data plane folder when using exclude paths, it won't cause any real problem.

Please share your thoughts, thanks.

@weshaggard
Copy link
Member

weshaggard commented Feb 5, 2020

Given the split nature of the the repository in all data-plane pipelines we use the include model to ensure we only start kicking off pipelines that are applicable to the changes. I personally think that is the right model because it doesn't seem like the correct trade-off to have other teams updating and dealing with pipelines that aren't applicable to them at all. Each data-plane service directory has its own pipeline and ci.yml and imagine if we switched all those to exclude filters instead. Who should be the one responsible for updating them? I don't think it should be the responsibility of everyone else working in the repo to update another yml file that are not even applicable to them which is why I prefer the include model which puts the responsibility on the owner to include the correct library paths and maintain their pipeline.

If you guys on-board a new service it should be pretty clear whether or not the mgmt ci leg ran or not and if it didn't then you need to update the include list.

Looking more into the future I'd like to eliminate the mgmt pipeline as it is today and align on a common pipeline model between both mgmt and data-plane the only reason I'm not pushing on that right now is because the mgmt libraries still use specialized tooling which I'd also like to eliminate. Ultimately I'd like to make the entire engineering system be exactly the same for all the libraries in this repo so we don't have to duplicate efforts across our teams.

@weshaggard
Copy link
Member

Is it possible to use wildcards in include such as sdk//Microsoft.Azure.Management.?

See https://docs.microsoft.com/en-us/azure/devops/pipelines/build/triggers?view=azure-devops&tabs=yaml#wildcards unfortunately it doesn't support that level of wildcards.

@chidozieononiwu chidozieononiwu force-pushed the ExcludeMgmtPipelinefromClientRuns branch 2 times, most recently from f01e3e7 to 36d918f Compare February 5, 2020 21:44
@chidozieononiwu chidozieononiwu force-pushed the ExcludeMgmtPipelinefromClientRuns branch from 36d918f to 25063b7 Compare February 5, 2020 21:47
@erich-wang
Copy link
Member

Given the split nature of the the repository in all data-plane pipelines we use the include model to ensure we only start kicking off pipelines that are applicable to the changes. I personally think that is the right model because it doesn't seem like the correct trade-off to have other teams updating and dealing with pipelines that aren't applicable to them at all. Each data-plane service directory has its own pipeline and ci.yml and imagine if we switched all those to exclude filters instead. Who should be the one responsible for updating them? I don't think it should be the responsibility of everyone else working in the repo to update another yml file that are not even applicable to them which is why I prefer the include model which puts the responsibility on the owner to include the correct library paths and maintain their pipeline.

If you guys on-board a new service it should be pretty clear whether or not the mgmt ci leg ran or not and if it didn't then you need to update the include list.

Looking more into the future I'd like to eliminate the mgmt pipeline as it is today and align on a common pipeline model between both mgmt and data-plane the only reason I'm not pushing on that right now is because the mgmt libraries still use specialized tooling which I'd also like to eliminate. Ultimately I'd like to make the entire engineering system be exactly the same for all the libraries in this repo so we don't have to duplicate efforts across our teams.

@weshaggard , thank you for sharing detail info, it sounds fine.
@isra-fel , could you please update .net sdk on boarding document to include this change, i.e. running update-mgmt-yml.ps1 for on boarding new mgmt sdk?

@chidozieononiwu chidozieononiwu merged commit 6616b68 into Azure:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants