Skip to content

DONT MERGE test exclude path on CI #58711

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.

Sign up for GitHub

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

Jump to bottom
Closed
Youssef1313 wants to merge 7 commits into from
Closed

DONT MERGE test exclude path on CI #58711

Youssef1313 wants to merge 7 commits into from

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jan 7, 2022

No description provided.

@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Infrastructure labels Jan 7, 2022
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 7, 2022

Related to #58710

@jaredpar I think this PR is merge-able with roslyn-CI being required and skipped. So it seems like we can just exclude whatever directories/files we don't want to trigger CI for.

@Youssef1313 Youssef1313 mentioned this pull request Jan 7, 2022
Merged
@jaredpar
Copy link
Member

jaredpar commented Jan 7, 2022

In general you cannot use excludes in our CI system. The reason is that many of our checks are required and GitHub interprets that as "required to pass". If you have an excluded file, make a change and submit a PR then the checks do not run at all. That is not considered passing hence the PR is red.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 7, 2022

@jaredpar The PR is currently red due to non-required checks I believe.

@Youssef1313 Youssef1313 marked this pull request as ready for review January 7, 2022 15:36
@Youssef1313 Youssef1313 requested a review from a team as a code owner January 7, 2022 15:36
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 7, 2022

@jaredpar Can you see if the merge button is available after an approval? (without actually merging)

@Youssef1313 Youssef1313 changed the title test exclude path on CI DONT MERGE test exclude path on CI Jan 7, 2022
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 7, 2022 •
edited
Loading

@jaredpar Here it is. It's green :)

This is what I'm seeing at this PR currently. I hope GitHub isn't showing something else to people with write access.

image

@jaredpar
Copy link
Member

jaredpar commented Jan 7, 2022

Interesting ... it is green now. I'm wondering if something changed. This did not used to be the case. That is why we've never used exclusions before, it basically had zero value with required checks. It does seem to be working now though.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jan 7, 2022

Yup I think GitHub behavior has changed at some point of time. Let's close this dummy PR and try to get #58712 merged 🎉

@Youssef1313 Youssef1313 closed this Jan 7, 2022
@Youssef1313 Youssef1313 deleted the patch-9 branch January 7, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar approved these changes

Assignees

No one assigned

Labels

Area-Infrastructure Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Youssef1313 @jaredpar