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

Verify that every commit exists on master during release cut #32422

Closed
wants to merge 4 commits into from

Conversation

jridgewell
Copy link
Contributor

This is my attempt to ensure we never have another bad cherry-pick release again (see Failures of versioning recent cherry-picks).

It's extremely important that the version assigned to cherry-pick requests is seen as a continuation of the base release (only the last 3 digits may change). If a new cherry-pick of a beta were to be seen as a brand new release, it would break the monotonic increase we need for npm publishing.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Feb 4, 2021

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js

@estherkim
Copy link
Collaborator

The unexpected amp version occurred because the cherry picked commits were resolving conflicts.

I thought this was a use case that we wanted to support?

@jridgewell
Copy link
Contributor Author

I thought this was a use case that we wanted to support?

By default, no. If there's something fishy, I'd rather halt than continue. But there's the --skip_commits_verification and --version_override that will allow us to support merge conflicts directly once we know about it.

@jridgewell
Copy link
Contributor Author

Ping @estherkim

@estherkim
Copy link
Collaborator

there's the --skip_commits_verification and --version_override that will allow us to support merge conflicts directly

That should work, as long as we can use that in our release automation process - @danielrozenberg is that possible?

@jridgewell
Copy link
Contributor Author

That should work, as long as we can use that in our release automation process - @danielrozenberg is that possible?

This is only needed for merge conflicts in CPs, which have to be done manually. All that's needed are documentation changes. The automation process should remain unchanged.

@@ -463,4 +484,7 @@ release.flags = {
' Directory path to emplace release files (defaults to "./release")',
'flavor':
' Limit this release build to a single flavor. Can be used to split the release work between multiple build machines.',
'skip_commits_verification':
' Skip verifying that every commit exists on master (or is a cherry-pick of a commit on master). If this is set, then you must provide version_override.',
'version_override': ' Overrides the version written to AMP_CONFIG',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it isn't enough to just have this flag, and discard the other flag. In either case we'll have to change the release automation backend to support this and to add a field to add the overridden version number for these releases, so I wouldn't want to merge this PR before the backend supports it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's another solution that doesn't require version_override or backend changes - #32689

Copy link
Member

Choose a reason for hiding this comment

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

@estherkim your PR seems fine by me (modulo the comment I left there, which is definitely not a blocker) but I think this current PR could still be relevant

@estherkim
Copy link
Collaborator

is this PR a moot poot now?

@CLAassistant
Copy link

CLAassistant commented Jul 22, 2022

CLA assistant check
All committers have signed the CLA.

@stale
Copy link

stale bot commented Oct 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Oct 15, 2023
@stale stale bot closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Inactive for one year or more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants