feat: detect if a release is necessary #750

nelsonfncosta · 2021-04-23T11:46:07Z

This PR tries to address the issue described in #192 in a way that is a non BREAKING CHANGE, the approach here is:

Expose a new command option that enables this behavior --detect-release
Get a list of commit since the latest git tag, via git-log-parser
Check if there is any commit type worthy of a release(ex: feat), this is done by taking advantage of the configuration files(ex: .versionrc), if no commit worthy is found we skip the release stage.

You can test this by doing a commit and then running the release command
ex:

...
git commit -m "docs: update README"
yarn release --detect-release

Demo

clog-sv-release.mov

Notes

Relevant links:
https://github.com/bendrucker/git-log-parser
https://github.com/sindresorhus/get-stream

Afterthought:
As mentioned in the issue thread this behavior could eventually become a default, and a --force option could be added to give users the ability to always create a new release commit

Closes #192

nelsonfncosta · 2021-04-23T11:58:04Z

Hey guys 👋
I came across issue #192, and I tried to follow some ideas/suggestions in the thread.
This is somewhat a proof of concept, and also an opportunity for me to explore the codebase 😅 .
I'm ok with this being replaced/closed in favor of --force options as mentioned in the issue.

Feedback is appreciated Thanks! 🙏

lib/utils.js

satazor · 2021-04-23T15:24:49Z

index.js

+      console.info(
+        chalk.green(`No commits found for types: [ ${releaseTypes.join(', ')} ], skipping release stage.`)
+      )
+      return 0


Why return 0 here? Also, I don't know if we should error out or exit successfully in this case. I'm leaned towards an error so that the process exists with != 0.

The intention with the return 0 was to say "script executed and exited successfully", I could replace it in favor of process.exit() 💭

Also, I don't know if we should error out or exit successfully in this case.

My perception of the issue at hand was that we didn't want to throw any error, what I understood from the initial issue was a use-case like "I have CI that always runs standard-version when a new commit is done, but it overpopulated some registry with unnecessary releases since some commits could be just docs or styles "

I'm concerned that throwing an error could be something unnecessary alarming for the pipeline, but we can do that if other people agree. 💭 💭

Closes conventional-changelog#192

codecov-commenter · 2021-04-26T16:10:05Z

Codecov Report

Merging #750 (4383381) into master (3d341c2) will decrease coverage by 0.23%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   97.75%   97.51%   -0.24%     
==========================================
  Files          24       25       +1     
  Lines        1067     1129      +62     
==========================================
+ Hits         1043     1101      +58     
- Misses         24       28       +4

Impacted Files	Coverage Δ
command.js	`84.61% <ø> (ø)`
defaults.js	`100.00% <ø> (ø)`
lib/lifecycles/bump.js	`94.50% <82.60%> (-4.05%)`	⬇️
lib/checkpoint.js	`100.00% <100.00%> (ø)`
test/bump.spec.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d341c2...4383381. Read the comment docs.

bcoe

I like this feature, thank you for the contribution 👍

My preference would be if we could figure out a way to detect-release, without pulling in new deps, I feel like we should be able to pull it off with some of the helpers already in the conventional-changelog monorepo.

bcoe · 2021-04-26T16:11:21Z

index.js

@@ -33,6 +35,23 @@ module.exports = async function standardVersion (argv) {
    }
  }

+  if (argv.detectRelease) {


I would be tempted to pull this into a detectRelease helper method.

Just to confirm, are you suggesting to move the logic we have inside this if to it's own file and expose a detectRelease method that we could call here?

@nelsonfncosta yes, I think it would make sense in its own helper method.

bcoe · 2021-04-26T16:12:52Z

package.json

@@ -60,6 +60,8 @@
    "eslint-plugin-import": "^2.22.1",
    "eslint-plugin-node": "^11.1.0",
    "eslint-plugin-promise": "^4.2.1",
+    "get-stream": "^6.0.1",
+    "git-log-parser": "^1.2.0",


I think we could probably use conventional-recommended-bump, which is already pulled in as a dependency, and avoid pulling in new get-stream and git-log-parser deps.

Quickly looking at the project, it seems to only expose a releaseType, do you think for a scenario where we only have commits with type docs , conventional-recommended-bump will return a relaseType of null/undefined opposed to "patch"?

Nonetheless, I will take a closer look into it!

ok, I tested the following scenarios
having the following commit history examples:

docs: update README (tag: v9.2.0) chore: release 9.2.0 (#736)

running conventional-recommended-bump -p angular would result in patch

feat: add cool stuff docs: update README (tag: v9.2.0) chore: release 9.2.0 (#736)

running conventional-recommended-bump -p angular would result in minor

So using the conventional-recommended-bump directly does not seem to help us

Looking at options we might be able to take advantage of whatBump

What do you think of checking potential releases in the whatBump callback, the commits argument given to it seems to be equivalent information to what we get from latest-commits.js, so we would only need to check for release worthy commit and exit if none found (what we do with the detectRelease arg).

standard-version/index.js

Lines 47 to 52 in 280c4c0

if (!releaseAvailable) {

console.info(

chalk.green(`No commits found for types: [ ${releaseTypes.join(', ')} ], skipping release stage.`)

)

process.exit()

}

💭 💭 💭

@nelsonfncosta I like the idea of trying to use whatBump, we already do the parsing in the various parts of the conventional changelog repo, so I'm hesitant to pull in any other deps -- you might also be able to leverage some of the dependencies used within the conventional changelog monorepo already?

@bcoe, can you take another look when possible 🙏

I was able to use whatBump and avoid pulling new dependencies, I split the new implementation into a separate commit to be easier to compare and see what changed.

bcoe · 2021-04-26T16:13:08Z

test/utils.spec.js

+    let result
+
+    beforeEach(async () => {
+      shell.exec('git commit --allow-empty -m "first-commit"')


I appreciate the thorough tests 👍

Previously we were using `args` to get the dryRun arg, but it seems to be a typo since the `args` is expected to be an array

nelsonfncosta · 2021-05-02T18:16:11Z

lib/checkpoint.js

@@ -3,7 +3,7 @@ const figures = require('figures')
 const util = require('util')

 module.exports = function (argv, msg, args, figure) {
-  const defaultFigure = args.dryRun ? chalk.yellow(figures.tick) : chalk.green(figures.tick)
+  const defaultFigure = argv.dryRun ? chalk.yellow(figures.tick) : chalk.green(figures.tick)


Just wanted to highlight this change, args.dryRun seems to have been a typo.
Looking at the usage of args the expected is an array and not an object.

standard-version/lib/checkpoint.js

Line 8 in 4383381

console.info((figure || defaultFigure) + ' ' + util.format.apply(util, [msg].concat(args.map(function (arg) {

bcoe

Sorry for additional nit, this is looking most of the way there.

bcoe · 2021-05-03T23:01:43Z

lib/lifecycles/bump.js

+          let breakings = 0
+          let features = 0
+
+          commits.forEach(commit => {


I think this is most of the way there, but one concern, the conventional-commits strategy has a more complex whatBump implementation:

https://github.com/conventional-changelog/conventional-changelog/blob/8076d4666c2a3ea728b95bf1e4e78d4c7189b1dc/packages/conventional-changelog-conventionalcommits/conventional-recommended-bump.js

Which includes support for ! in the commit body.

Is it possible for us to use whatever default whatBump is configured already, and then update the reason afterwards?

Initially, I had that approach in mind but after some exploring, I was not able to find a clear way to get the default whatBump implementation, ending up implementing my custom whatBump :/

💭 Would be cool if conventional-recommended-bump passed the default whatBump callback as an option argument to the whatBump option, whatBump(commits, { defaultWhatBump, ...}) {}; 💭
It sends a bunch of options but none we can use.

How do you think we should proceed @bcoe ?
I can adapt the current implementation to follow the strategy you linked, to support the ! use case you mentioned.

@nelsonfncosta I think each preset should be exporting its recommendedBump configuration, see:

https://github.com/conventional-changelog/conventional-changelog/blob/8076d4666c2a3ea728b95bf1e4e78d4c7189b1dc/packages/conventional-changelog-conventionalcommits/index.js#L29

The problem with copying the logic over from any one preset, is that we allow folks to configure their own preset -- so although the logic would be good for conventional commits, it would be off potentially for another plugin.

I haven't tried to do this myself, and apologize for how much of a yarn ball the conventional changelog codebase is -- I think though you should be able to pluck whatBump off of a given preset.

I will take a closer look into this! 👀
The information we need is on the recommendedBumpOpts, so if this is being exported it should be a straightforward change 👍

I have been looking for a way to get the preset config, they are not directly exported
something along the lines

const config = require('conventional-changelog-conventionalcommits') const { recommendedBumpOpts } = config(args)

the problem I'm facing I'm not really sure how to "wait/get" my value from here

Vyazovoy · 2022-01-12T17:01:34Z

Would be really good to merge this PR as it fixes significant flaw.

patoncrispy · 2022-04-11T08:29:11Z

Hey there, just wondering if there is much left in this PR? Is there help needed getting this across the line?

paulobmarcos · 2022-05-11T09:39:18Z

Thanks @nelsonfncosta for having a go at this 🙇 Would be really a nice option to have!

Is there anything else missing for this to be included? Is the problem mentioned on this comment the current blocker?

satazor suggested changes Apr 23, 2021

View reviewed changes

lib/utils.js Outdated Show resolved Hide resolved

lib/utils.js Outdated Show resolved Hide resolved

nelsonfncosta force-pushed the master branch 2 times, most recently from d02f377 to 7b9defb Compare April 23, 2021 14:14

satazor reviewed Apr 23, 2021

View reviewed changes

feat: detect if a release is necessary

280c4c0

Closes conventional-changelog#192

nelsonfncosta force-pushed the master branch from 7b9defb to 280c4c0 Compare April 23, 2021 15:58

bcoe changed the title ~~Detect if a release is necessary~~ feat: detect if a release is necessary Apr 26, 2021

bcoe requested changes Apr 26, 2021

View reviewed changes

nelsonfncosta added 2 commits May 2, 2021 19:03

fix(checkpoint): use argv to get dryRun arg

bf86499

Previously we were using `args` to get the dryRun arg, but it seems to be a typo since the `args` is expected to be an array

refactor: use conventional-recommended-bump to support --detect-release

4383381

nelsonfncosta commented May 2, 2021

View reviewed changes

nelsonfncosta requested a review from bcoe May 3, 2021 08:52

bcoe approved these changes May 3, 2021

View reviewed changes

bcoe requested changes May 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: detect if a release is necessary #750

feat: detect if a release is necessary #750

nelsonfncosta commented Apr 23, 2021 •

edited

Loading

nelsonfncosta commented Apr 23, 2021

satazor Apr 23, 2021

nelsonfncosta Apr 23, 2021 •

edited

Loading

codecov-commenter commented Apr 26, 2021 •

edited

Loading

bcoe left a comment

bcoe Apr 26, 2021

nelsonfncosta Apr 26, 2021 •

edited

Loading

bcoe Apr 28, 2021

bcoe Apr 26, 2021

nelsonfncosta Apr 26, 2021 •

edited

Loading

nelsonfncosta Apr 28, 2021 •

edited

Loading

nelsonfncosta Apr 28, 2021 •

edited

Loading

bcoe Apr 28, 2021

nelsonfncosta May 2, 2021

bcoe Apr 26, 2021

nelsonfncosta May 2, 2021

bcoe left a comment

bcoe May 3, 2021

nelsonfncosta May 4, 2021 •

edited

Loading

bcoe May 4, 2021

nelsonfncosta May 4, 2021

nelsonfncosta May 15, 2021 •

edited

Loading

Vyazovoy commented Jan 12, 2022

patoncrispy commented Apr 11, 2022

paulobmarcos commented May 11, 2022

	if (!releaseAvailable) {
	console.info(
	chalk.green(`No commits found for types: [ ${releaseTypes.join(', ')} ], skipping release stage.`)
	)
	process.exit()
	}

feat: detect if a release is necessary #750

Are you sure you want to change the base?

feat: detect if a release is necessary #750

Conversation

nelsonfncosta commented Apr 23, 2021 • edited Loading

Demo

Notes

nelsonfncosta commented Apr 23, 2021

Choose a reason for hiding this comment

nelsonfncosta Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 26, 2021 • edited Loading

Codecov Report

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonfncosta Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonfncosta Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

nelsonfncosta Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

nelsonfncosta Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonfncosta May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonfncosta May 15, 2021 • edited Loading

Choose a reason for hiding this comment

Vyazovoy commented Jan 12, 2022

patoncrispy commented Apr 11, 2022

paulobmarcos commented May 11, 2022

nelsonfncosta commented Apr 23, 2021 •

edited

Loading

nelsonfncosta Apr 23, 2021 •

edited

Loading

codecov-commenter commented Apr 26, 2021 •

edited

Loading

nelsonfncosta Apr 26, 2021 •

edited

Loading

nelsonfncosta Apr 26, 2021 •

edited

Loading

nelsonfncosta Apr 28, 2021 •

edited

Loading

nelsonfncosta Apr 28, 2021 •

edited

Loading

nelsonfncosta May 4, 2021 •

edited

Loading

nelsonfncosta May 15, 2021 •

edited

Loading