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

refactor(bump-all-updated-packages): use tag instead of custom commit message #36220

Closed
wants to merge 1 commit into from

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Feb 20, 2023

Summary

Having custom commit message script is not an option for main branch, because we have internal tooling, which strips [x] tags from commit messages before merging them into main branch.

Instead of constant commit message, we are now using a tag which will be concatenated with the commit message, which was entered via interactive commit dialog, see demo below.

Changelog

[Internal] - updated validation in bumping packages script

Test Plan

Screen.Recording.2023-02-20.at.16.49.44.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Feb 20, 2023
@facebook-github-bot
Copy link
Contributor

@hoxyq has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hoxyq hoxyq force-pushed the packages-ci/update-validation branch from b39c08b to 91fc8de Compare February 20, 2023 17:04
@facebook-github-bot
Copy link
Contributor

@hoxyq has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Comment on lines +82 to +83
const hasSpecificPublishTag =
commitMessage.includes(PUBLISH_PACKAGES_TAG);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would maybe keep both logic in OR, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we keep both, but cli (npm run bump-all-updated-packages) will only append @publish-... tag, this means that some users might put this commit message manually

I think its better to avoid this and keep cli as the only right way to do packages bumping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yeah, I understand that user can still append this tag manually and this will trigger the publishing process

@hoxyq hoxyq force-pushed the packages-ci/update-validation branch 2 times, most recently from 72bfae8 to 6badfee Compare February 20, 2023 17:25
@facebook-github-bot
Copy link
Contributor

@hoxyq has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kelset
Copy link
Contributor

kelset commented Feb 20, 2023

(small sidenote: let's make sure to update the wiki to reflect the latest once this is merged)

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,451,960 +7
android hermes armeabi-v7a 7,775,340 +7
android hermes x86 8,927,563 +1
android hermes x86_64 8,785,165 +0
android jsc arm64-v8a 6,668,753 +0
android jsc armeabi-v7a 7,462,720 +0
android jsc x86 9,192,880 +0
android jsc x86_64 6,893,882 +0

Base commit: 5d6f21d
Branch: main

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 21, 2023
@facebook-github-bot
Copy link
Contributor

@hoxyq merged this pull request in 18b402c.

@cortinico
Copy link
Contributor

Hey @hoxyq,
Just a heads up that I believe this won't work sadly. Specifically Phabricator seems to strip @ away when exporting Diffs.

For example D43619512 from @cipolleschi had an @allow-large-files which got stripped on the final commit 05438c3

So we'd need to either search for publish-packages-to-npm or use [publish-packages-to-npm] or any other notation

@hoxyq
Copy link
Contributor Author

hoxyq commented Feb 28, 2023

Hey @hoxyq, Just a heads up that I believe this won't work sadly. Specifically Phabricator seems to strip @ away when exporting Diffs.

For example D43619512 from @cipolleschi had an @allow-large-files which got stripped on the final commit 05438c3

So we'd need to either search for publish-packages-to-npm or use [publish-packages-to-npm] or any other notation

Thanks for highlighting this, I will check if Phabricator strips only @, maybe we could use ~publish-packages-to-npm or #publish-packages-to-npm

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
… message (facebook#36220)

Summary:
Having custom commit message script is not an option for `main` branch, because we have internal tooling, which strips `[x]` tags from commit messages before merging them into `main` branch.

Instead of constant commit message, we are now using a tag which will be concatenated with the commit message, which was entered via interactive commit dialog, see demo below.

## Changelog
[Internal] - updated validation in bumping packages script

Pull Request resolved: facebook#36220

Test Plan: https://user-images.githubusercontent.com/28902667/220163767-015bf37b-6914-4df2-84d9-aa25fb2887d3.mov

Reviewed By: cortinico

Differential Revision: D43443597

Pulled By: hoxyq

fbshipit-source-id: 08e5e08524a1d934fbb35529e025358d7bf3b203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants