-
Notifications
You must be signed in to change notification settings - Fork 186
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
Feature/eng common patch 2 #1052
Feature/eng common patch 2 #1052
Conversation
97964d7
to
c9158f8
Compare
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.
Quite a few moving parts with this one. Two comments related to security.
7772207
to
386c3e1
Compare
386c3e1
to
f63aeba
Compare
|
||
- task: PowerShell@2 | ||
displayName: Queue test pipeline | ||
condition: and(succeeded(), ne(variables['${{repo}}-template-definition-id'], '')) |
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.
Where do these definition id's get setup? Is it only in the pipeline itself?
Should this be part of the sync-directory.yml file or the eng-common-sync.yml? It feels like it should be part of the eng-common-sync.yml file but I think we are starting to make this sync-directory.yml file be less useful for anything else so maybe it doesn't matter.
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.
I did not want to dump the pipeline definition IDs in the source. I know you weren't ok with that in some of my previous work, so I put it in the pipeline via the UI.
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.
I don't particularly like them in either location :) However I was mostly curious about where this lives. Is there a way to look up the definition id by name? So we could build a name and then try to find the associated definition.
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.
It looks like one still requires the Definition ID to request information about the pipeline. https://docs.microsoft.com/en-us/rest/api/azure/devops/pipelines/pipelines/get?view=azure-devops-rest-6.1
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.
Sadly the API you need is split across multiple groups of REST API, you can use this one to look up a pipeline definition by name:
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.
@chidozieononiwu can you please file a follow-up item to figure out a better way to configure the list of test pipelines we want to trigger? I think I'd prefer a list of pipelines in the yml file over having them be hidden away in the variables tab in the web UI.
3989dc8
to
130c5aa
Compare
130c5aa
to
a0e6990
Compare
The following pipelines have been queued for testing: |
a0e6990
to
30cfbd6
Compare
30cfbd6
to
0690b24
Compare
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
/check-enforcer override |
* Update Tools repo Sync to make use of patch files rather than replacing all files in the sync directory * Update location of patch files * Add some changes in eng/common to test things * Update patch application logic * Switch to pushing to upstream * Change workflow to use upstream branches * Add step tp delete braches after merging
Includes changes from #995
But with a new workflow that delays the creation of sync pull requests until test pipelines have been run successfully.