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

Make build-system code main-branch-agnostic #33535

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Make build-system code main-branch-agnostic #33535

merged 2 commits into from
Mar 31, 2021

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Mar 29, 2021

PR Highlights:

  • Remove all hard-coded references to master in the build-system/ implementation
  • Add a single source of truth in common/main-branch.js (will be changed to main later)

Notes:

  • As written, this PR should be a no-op.
  • Documentation / comment URLs in our codebase will be changed in a follow up PR.

Partial fix for #32195

@amp-owners-bot
Copy link

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js
build-system/tasks/visual-diff/index.js

@danielrozenberg
Copy link
Member

This seems like overkill for me... Are we expecting this value to change more than once? Introducing another dependency (even if it's a single string) makes the code slightly more difficult to read in places where there's no real reason to once the switch is made...

@rsimha
Copy link
Contributor Author

rsimha commented Mar 29, 2021

This seems like overkill for me... Are we expecting this value to change more than once? Introducing another dependency (even if it's a single string) makes the code slightly more difficult to read in places where there's no real reason to once the switch is made...

This is being done in order to make the migration safe and gradual. After this PR lands, we'll need to rename the branch on GitHub and then immediately change this file and force-merge the change. Trying to coordinate all changes is inherently risky and can break CI / development if even one merge conflict creeps in. Long-term, I agree that main-branch.js can be cleaned up.

@estherkim
Copy link
Collaborator

I agree with @danielrozenberg. I think a find&replace should be fine, as long as we have the release automation changes ready to go too.

I found this useful https://www.hanselman.com/blog/easily-rename-your-git-default-branch-from-master-to-main

@rsimha
Copy link
Contributor Author

rsimha commented Mar 29, 2021

I agree with @danielrozenberg. I think a find&replace should be fine, as long as we have the release automation changes ready to go too.

To clarify, Is the main (heh!) objection the addition of main-branch.js? Or is there more to it? Right now, there are 7 spots across build-system/ that were hard-coding master and are now referencing this file. The idea of consolidating all 7 to one source of truth is that it can be trivially updated and force-merged when the switchover happens without breaking CI or development.

Trying to coordinate this change with other changes across amphtml source code, amp-github-apps source code, documentation files, github admin settings, release automation, and not sure what else is something I'd like to simplify as much as possible. I do plan on cleaning this up once the migration is complete and the dust settles, but would like to avoid the additional burden of having to instantly update a large number of spots via find & replace (after this PR, there will be a few dozen left in build-system/ and O(thousand) left in amphtml).

Let's discuss further during tomorrow's Infra stand-up. There might be other ideas that haven't been considered.

I found this useful https://www.hanselman.com/blog/easily-rename-your-git-default-branch-from-master-to-main

Agree, this is useful. However, it looks that the main branch created with these instructions will not keep up with new changes on master, which is something that our CI and custom apps rely on.

@rsimha
Copy link
Contributor Author

rsimha commented Mar 30, 2021

As discussed offline, the goal is to use main-branch.js during the transition to make it easier, and to remove it soon after everything has moved over. I've also written up a doc to outline next steps. (Clean up is part of the plan.)

@rsimha
Copy link
Contributor Author

rsimha commented Mar 31, 2021

@ampproject/wg-infra Any remaining comments to address? Okay to merge this part?

build-system/common/main-branch.js Outdated Show resolved Hide resolved
build-system/pr-check/visual-diff-tests.js Show resolved Hide resolved
@rsimha rsimha merged commit 9beb76a into ampproject:master Mar 31, 2021
@rsimha rsimha deleted the 2021-01-26-MainMaster branch March 31, 2021 18:41
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants