-
Notifications
You must be signed in to change notification settings - Fork 27
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
T/272: Changed order of calling commands (release tool) #281
Conversation
log.info( `Synchronizing a local repository with the remote for "${ packageJson.name }"...` ); | ||
|
||
// Push updated dependencies and changelog if was generated. | ||
exec( 'git pull && git push' ); |
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.
We can't pull after tagging a new version. That would break the tag.
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.
Sure. Pull should be done before tagging.
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.
Those commands should not try to pull anything. The assumption is that no one will do any changes during the process. In other words – let's assume that the releaser pulled everything, (optionally) validate that there are no new changes before committing anything, and then just push.
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.
I removed pulling and didn't introduce your optionally validation step.
Let's assume that everything is pulled before starting release.
@@ -208,28 +220,79 @@ module.exports = function releaseSubRepositories( options ) { | |||
} ); | |||
} | |||
|
|||
function releasePackages( releaseOptions ) { | |||
function pullAndPushPackages() { |
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.
Why is this function located before bumpVersion()
? Sort the code in the order your read it.
const displaySkippedPackages = require( '../utils/displayskippedpackages' ); | ||
const executeOnPackages = require( '../utils/executeonpackages' ); | ||
const generateChangelogForSinglePackage = require( './generatechangelogforsinglepackage' ); | ||
const getPackageJson = require( '../utils/getpackagejson' ); | ||
const getPackagesToRelease = require( '../utils/getpackagestorelease' ); | ||
const getSubRepositoriesPaths = require( '../utils/getsubrepositoriespaths' ); | ||
const releaseRepository = require( '../utils/releaserepository' ); |
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.
If the new code in this file does all the releasing why haven't you removed the releaseRepository
file?
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.
We still have a task for releasing the single repository which uses this util.
@Reinmar, Could you review once again? |
All worked great. I commented out |
Suggested merge commit message (convention)
Fix: Changed order of commands in release tool. Closes #272. Closes #292.
Due to releasing packages one after another, the CI could break the builds. Now release tool will:
Function
validatePackageToRelease()
will not return an error if the branch is ahead with the remote.Additional information
I had to change the
validatePackageToRelease()
function because commit with updating dependencies cannot be pushed before releasing packages on npm.