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

[ci] Stop building monodroid in builds from forks #8444

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

pjcollins
Copy link
Member

Removes all usage of provisionator and monodroid build and test steps
when building a PR from a fork.

@pjcollins pjcollins force-pushed the oss-forks branch 3 times, most recently from de53fef to 3078a28 Compare October 23, 2023 16:40
Removes all usage of provisionator and monodroid build and test steps
when building a PR from a fork.
@pjcollins
Copy link
Member Author

Tagging everyone for review for awareness, once this lands any xamarin/monodroid related changes will need to come from xamarin-android branches.

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

The changes look good.

One concern is: are we worried about accidentally merging a PR that was from a fork and thus has not had the full battery of checks run?

If we are, we could write a step that fails if running from a fork, so we would at least always see an error that we could choose to ignore if we want.

I will leave this decision up to others, as I do not use forks. 😁

Copy link
Contributor

@moljac moljac left a comment

Choose a reason for hiding this comment

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

LGTM

@dellis1972
Copy link
Contributor

This looks good.

What is the process for dealing with Third party contributions? they won't have the permissions to push to the main repo, so I guess we will need to merge PR's and then fix issues with the full build if they appear?

@pjcollins
Copy link
Member Author

This looks good.

What is the process for dealing with Third party contributions? they won't have the permissions to push to the main repo, so I guess we will need to merge PR's and then fix issues with the full build if they appear?

Yeah this sounds about right, we will need to monitor our regular CI results a bit more closely to make sure it stays healthy after this.

@grendello
Copy link
Contributor

grendello commented Oct 24, 2023

This looks good.
What is the process for dealing with Third party contributions? they won't have the permissions to push to the main repo, so I guess we will need to merge PR's and then fix issues with the full build if they appear?

Yeah this sounds about right, we will need to monitor our regular CI results a bit more closely to make sure it stays healthy after this.

Should we perhaps change the default branch to dev and make main "ready for release"? 3rd party PRs would be merged against dev and, if they don't break the branch, cherry-picked to main. It's more work, but at least main wouldn't be potentially broken for anyone pulling changes in the morning should a PR break something. Our "internal" PRs could still be made against main, since they'd be from branches in this repository and thus fully tested.

@pjcollins
Copy link
Member Author

The idea of adding an intermediate branch such as dev seems interesting, though I'm not sure the overhead of keeping the two branches in sync and merging back and forth is doing to be worth it? I think in the vast vast majority of cases we only ship from release branches (such as release/8.0.1xx-rc2), so we have a bit of a "buffer" in place already.

@grendello
Copy link
Contributor

@pjcollins I'm more worried about timezones here - if someone in the US or Europe or anywhere else merges a broken 3rd party PR, then people waking up in the morning, while the committer isn't available, will be faced with a broken branch after they pull changes at the start of their day. We have (unfortunately) so few 3rd party PRs that the overhead shouldn't be too much.

@pjcollins
Copy link
Member Author

This PR still ran 6,676 tests compared to the 6,754 tests in a recent build from main, I am hoping that the scenario where a forked build causes a real breakage is going to be extremely small. We can look at workflow changes in the future if we do start running into issues.

@pjcollins pjcollins merged commit b1bb67b into dotnet:main Oct 24, 2023
47 checks passed
@pjcollins pjcollins deleted the oss-forks branch October 24, 2023 18:00
grendello added a commit that referenced this pull request Oct 24, 2023
* main:
  [ci] Stop building monodroid in builds from forks (#8444)
  [docs] Don't overwrite xml produced by docs build (#8451)
  Bump NDK to r26b (#8450)
  [Tests] Fix TestResolveToolsExists on .NET (#8445)
pjcollins added a commit that referenced this pull request Nov 1, 2023
Removes all usage of provisionator and monodroid build and test steps
when building a PR from a fork.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants