-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Set the last three digit of the AMP version to be the number of cherry-picks in the branch #27848
Set the last three digit of the AMP version to be the number of cherry-picks in the branch #27848
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for embarking on this venture, @danielrozenberg 😃
A few comments below, and one suggestion to make versioning more predictable and eliminate the flag. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate you patiently dealing with all these review comments, @danielrozenberg. LGTM.
PS: Remember to modify the PR title to say "last three digits" instead of "last digit" 😃
@mdmower I would love an approval from you before I merge this! |
@danielrozenberg I appreciate the consideration! I think the idea is sound, but I have the same concern that @rsimha mentioned here. He seems to consider it a non-issue, so I still have to reconcile these results for myself:
To me this looks like an out of date local |
No rush! |
@danielrozenberg @rsimha I haven't resolved the dilemma I mentioned in my previous comment. Below is a simplified version. The issue I'm seeing is that an out of date local
In other words, the build system would identify zero cherry-picks using If you agree this is a concern, then you could switch to running
|
This is an amazing deep dive into the different ways one can use Git :) I think this is mostly a non-issue, as I doubt many people will bother doing a fetch from We enforce (both through automation and through due-process) an invariant that would prevent this from affecting actual releases - we reject releases that would return any I'll merge this PR now so that nightly builds starting next week will use this new format, we can always tweak the finer details later if you still think this is an edge case that should be addressed |
Just to clarify, I was providing steps for reproducing the issue. They not all necessary in practice. All that's necessary is for someone to run EDIT: In fact, here's a real world example: Simulate a stale master branch
Checkout a release tag and count the cherry-picks
In other words, because |
I see... Though I don't know why someone would do that :) |
I'm with @mdmower here, this can cause all kinds of problems. For instance, I don't frequently update my local As, is, I will always return the wrong number of cherry-picks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #27848 (comment), I think this should be reverted.
|
||
const lastCommitFormattedTime = gitCommitFormattedTime( | ||
`HEAD~${numberOfCherryPicks}` | ||
).slice(0, -2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the only caller. Why not just remove the %S
in the formatted time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jridgewell does it really matter to you what AMP version number shows up when you're doing local development? My main concern with this PR was fixing version numbers for releases, while making sure that local development will continue working uninterrupted (i.e., no errors or completely incomprehensible version numbers) |
I'm hopeful that #28035 will alleviate concern and this commit can be left in-place (no revert necessary). |
This isn't for local development. Our cherry-pick process boils down to: git clone git@github.com:ampproject/amphtml.git ampproject/amphtml
cd ampproject/amphtml
git checkout -b amp-release-2020-04-27 ${AMP_VERSION}
git cherry-pick -x ${SHA}
git push --set-upstream origin amp-release-2020-04-27 If my local |
That's why the first line there is |
Why would I clone the repo if I already have it? We should add an explicit step to |
I actually don't understand why the cherry-pick logic is looking at |
It guarantees that you have the cherry pick commit, which is usually a recently merged PR. |
Can you explain what you mean? |
Hmm well I'm not following the "diffing against the |
@jridgewell sorry to pile on to this discussion, but can you summarize your main objection to this PR? (Is the problem the originally stated goal? Or is it the
In order to identify which commits were cherry-picked from
See the example below. |
…y-picks in the branch (ampproject#27848)
Implements and closes #27793