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

T/272: Changed order of calling commands (release tool) #281

Merged
merged 10 commits into from
Oct 20, 2017
Merged

Conversation

pomek
Copy link
Member

@pomek pomek commented Sep 18, 2017

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:

  • do all commits (generate missing changelogs or/and update dependencies' versions),
  • publish all packages on NPM (all packages will contain proper versions),
  • do all pushes (CI will not crash because all versions are valid),
  • make the GitHub releases.

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.

@pomek pomek requested a review from Reinmar September 18, 2017 07:53
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' );
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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() {
Copy link
Member

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' );
Copy link
Member

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?

Copy link
Member Author

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.

@pomek
Copy link
Member Author

pomek commented Sep 27, 2017

@Reinmar, pull will be now done before the tagging.

Could you review once again?

@Reinmar
Copy link
Member

Reinmar commented Oct 20, 2017

All worked great. I commented out npm publish and git push calls and the rest went smooth!

@Reinmar Reinmar merged commit 451ff8c into master Oct 20, 2017
@Reinmar Reinmar deleted the t/272 branch October 20, 2017 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants