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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions scripts/monorepo/__tests__/find-and-publish-all-bumped-packages-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

const {spawnSync} = require('child_process');

const {BUMP_COMMIT_MESSAGE} = require('../constants');
const {PUBLISH_PACKAGES_TAG} = require('../constants');
const forEachPackage = require('../for-each-package');
const findAndPublishAllBumpedPackages = require('../find-and-publish-all-bumped-packages');

Expand All @@ -31,7 +31,7 @@ describe('findAndPublishAllBumpedPackages', () => {
}));

spawnSync.mockImplementationOnce(() => ({
stdout: BUMP_COMMIT_MESSAGE,
stdout: `This is my commit message\n\n${PUBLISH_PACKAGES_TAG}`,
}));

expect(() => findAndPublishAllBumpedPackages()).toThrow(
Expand Down
19 changes: 17 additions & 2 deletions scripts/monorepo/bump-all-updated-packages/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
*/

const chalk = require('chalk');
const {execSync} = require('child_process');
const inquirer = require('inquirer');
const path = require('path');
const {echo, exec, exit} = require('shelljs');
const yargs = require('yargs');

const {BUMP_COMMIT_MESSAGE} = require('../constants');
const {PUBLISH_PACKAGES_TAG} = require('../constants');
const forEachPackage = require('../for-each-package');
const checkForGitChanges = require('../check-for-git-changes');
const bumpPackageVersion = require('./bump-package-version');
Expand Down Expand Up @@ -167,7 +168,21 @@ const main = async () => {
return;
}

exec(`git commit -a -m "${BUMP_COMMIT_MESSAGE}"`, {cwd: ROOT_LOCATION});
// exec from shelljs currently does not support interactive input
// https://github.com/shelljs/shelljs/wiki/FAQ#running-interactive-programs-with-exec
execSync('git commit -a', {cwd: ROOT_LOCATION, stdio: 'inherit'});

const enteredCommitMessage = exec('git log -n 1 --format=format:%B', {
cwd: ROOT_LOCATION,
silent: true,
}).stdout.trim();
const commitMessageWithTag =
enteredCommitMessage + `\n\n${PUBLISH_PACKAGES_TAG}`;

exec(`git commit --amend -m "${commitMessageWithTag}"`, {
cwd: ROOT_LOCATION,
silent: true,
});
})
.then(() => echo());
}
Expand Down
4 changes: 2 additions & 2 deletions scripts/monorepo/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@
* @format
*/

const BUMP_COMMIT_MESSAGE = '[ci][monorepo] bump package versions';
const PUBLISH_PACKAGES_TAG = '@publish-packages-to-npm';

module.exports = {BUMP_COMMIT_MESSAGE};
module.exports = {PUBLISH_PACKAGES_TAG};
8 changes: 4 additions & 4 deletions scripts/monorepo/find-and-publish-all-bumped-packages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
const path = require('path');
const {spawnSync} = require('child_process');

const {BUMP_COMMIT_MESSAGE} = require('./constants');
const {PUBLISH_PACKAGES_TAG} = require('./constants');
const forEachPackage = require('./for-each-package');

const ROOT_LOCATION = path.join(__dirname, '..', '..');
Expand Down Expand Up @@ -79,10 +79,10 @@ const findAndPublishAllBumpedPackages = () => {
process.exit(1);
}

const hasSpecificCommitMessage =
commitMessage.startsWith(BUMP_COMMIT_MESSAGE);
const hasSpecificPublishTag =
commitMessage.includes(PUBLISH_PACKAGES_TAG);
Comment on lines +82 to +83
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


if (!hasSpecificCommitMessage) {
if (!hasSpecificPublishTag) {
throw new Error(
`Package ${packageManifest.name} was updated, but not through CI script`,
);
Expand Down