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

Warn PR author when eng/common is modified in consumer repo #6768

Open
am11 opened this issue Jan 9, 2021 · 6 comments
Open

Warn PR author when eng/common is modified in consumer repo #6768

am11 opened this issue Jan 9, 2021 · 6 comments

Comments

@am11
Copy link
Member

am11 commented Jan 9, 2021

In arcade-powered repos, eng/common is logically a (readonly) mirror directory, which dotnet-maestro bot dutifully updates.

If a PR is submitted by a human being that updates a file under this directory, it is easy to misremember during the PR review. That causes a merge conflict in the next arcade update and mostly results in reverting the manual changes made earlier.

It would be nice if a bot post a comment to such PRs to help reminding reviewers about readonly-ness of eng/common, something to the effect:

Reminder: eng/common is a mirror directory. Please submit changes in dotnet/arcade repo to avoid conflicts during the subsequent arcade update.

ps: such reverts took place in dotnet/runtime repo few times, and currently eng/common in aspnetcore repo is diverged.

@missymessa
Copy link
Member

There is currently a readme.md in eng/common that warns against making changes in that folder:

https://github.com/dotnet/arcade/blob/master/eng/common/README.md

@am11
Copy link
Member Author

am11 commented Jan 9, 2021

Great. I guess a common bot could point to this document in next line of that comment (See https://github.com/dotnet/arcade/blob/master/eng/common/README.md). If reviewers can forgot that eng/common is mirrored, it is likely that they will also forget to read the docs. :)

@garath
Copy link
Member

garath commented Jan 21, 2021

We don't really have that kind of infrastructure to proactively detect and correctly act on changes to eng/common. Usually, (intentional or accidental) changes caught and managed in the PR.

Certainly, if we see more issues in this area changes here we'll look at investing more.

@danmoseley
Copy link
Member

@akoeplinger I think you made the backport Github Action. do you know of any existing action that would do something like the above?

@am11
Copy link
Member Author

am11 commented Sep 13, 2021

In practice, only few people are aware of https://github.com/dotnet/arcade/blob/master/eng/common/README.md, so we end up sending three PRs:

  • send PR to dotnet/{downstream repo} modifying something in eng/common. since 99% people don't remember this arbitrary detail that eng/common is a mirror directory, it gets merged.
    • i personally don't think it's anyone's fault, just the case of incomplete automation or opportunity to improve the PR experience.
  • the next dotnet/arcade update PR overwrites the changes
  • author sends PR to dotnet/arcade
  • update downstream repo again (or wait for next infra update)

the fact is, this is not happening infrequently.

@akoeplinger
Copy link
Member

akoeplinger commented Sep 29, 2021

@danmoseley This should be really easy to do with a GitHub Action that gets triggered by changes to eng/common, we'd just need to no-op for PRs from dotnet-bot i.e. legitimate updates. I'll take a stab at it.

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

No branches or pull requests

6 participants