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

rules skipped if commit message contains '/' #175

Open
pheenomenon opened this issue May 21, 2020 · 7 comments
Open

rules skipped if commit message contains '/' #175

pheenomenon opened this issue May 21, 2020 · 7 comments

Comments

@pheenomenon
Copy link

The below release rules works well, until the commit message has a '/' in it.
For ex BUGFIX-RELEASE: wait to set up event emitter until calling /process
Here are the logs in this case:

[9:47:58 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
328[9:47:58 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: BUGFIX-RELEASE: wait to set up event emitter until calling /process (#15)
329
330* wait to set up event emitter until calling /process
334* add tests
335
336* small fixes
3372020-05-21T21:47:58.969Z semantic-release:commit-analyzer Analyzing with custom rules
3382020-05-21T21:47:58.971Z semantic-release:commit-analyzer Analyzing with default rules
339[9:47:58 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release

My release rules looks like this:

 ["@semantic-release/commit-analyzer", {
        "preset": "eslint",
        "releaseRules": [
        {
          "subject": "*",
          "release": false
        },
        {
          "subject": "FEATURE-RELEASE:*",
          "release": "minor"
        },
        {
          "subject": "BUGFIX-RELEASE:*",
          "release": "patch"
        },
        {
          "subject": "BREAKING-RELEASE:*",
          "release": "major"
        }
        ]
      }]
@no-creative-name
Copy link

no-creative-name commented Oct 23, 2020

What's the status here? I saw that the fix is not working and this bug currently prevents us from using semantic release properly.

@suhasgaddam-trueaccord
Copy link

The bug/consequence of this is from the underlying micromatch library used. Either a different library should be used, / removed from messages passed to micromatch, or a huge warning be placed in this repository. I would much prefer either of the first 2 options.

To reproduce the behavior:
https://runkit.com/60587ebc6a51e10013d391c9/60587ebcc774380013410cab

var micromatch = require("micromatch")
var picomatch = require("picomatch")
console.log("1", micromatch(['abc BUMP_MAJOR def'], ['**BUMP_MAJOR**']));
console.log("2", micromatch(['abc /BUMP_MAJOR def'], ['**BUMP_MAJOR**']));
console.log("3", micromatch(['abc / BUMP_MAJOR def'], ['**BUMP_MAJOR**']));
console.log("4", micromatch(['/abc BUMP_MAJOR def'], ['**BUMP_MAJOR**']));
console.log("5", micromatch(['abc BUMP_MAJOR def/'], ['**BUMP_MAJOR**']));
console.log("6", micromatch(['abc BUMP_MAJOR/def'], ['**BUMP_MAJOR**']));
console.log("7", micromatch(['abc BUMP_MAJOR/ def'], ['**BUMP_MAJOR**']));
console.log("8", micromatch(['/foo/'], ['**f**']));
console.log("1", picomatch('**BUMP_MAJOR**')('abc BUMP_MAJOR def'));
console.log("2", picomatch('**BUMP_MAJOR**')('abc /BUMP_MAJOR def'));
console.log("3", picomatch('**BUMP_MAJOR**')('abc / BUMP_MAJOR def'));
console.log("4", picomatch('**BUMP_MAJOR**')('/abc BUMP_MAJOR def'));
console.log("5", picomatch('**BUMP_MAJOR**')('abc BUMP_MAJOR def/'));
console.log("6", picomatch('**BUMP_MAJOR**')('abc BUMP_MAJOR/def'));
console.log("7", picomatch('**BUMP_MAJOR**')('abc BUMP_MAJOR/ def'));
console.log("8", picomatch('**f**')('/foo/', ));

@gr2m
Copy link
Member

gr2m commented Apr 1, 2021

The bug/consequence of this is from the underlying micromatch library used

can you file an issue with micromatch and reference it here?

@suhasgaddam-trueaccord
Copy link

@gr2m - I've been trying to figure out if micromatch is performing as expected or if it is a bug on their end. From reading their documentation and examples, the library feels like purpose meant for path matching. If that's the case, then commit-analyzer will need to switch to a different matching library.

@alexkli
Copy link

alexkli commented May 25, 2021

Would be great to fix this. We often have PRs/commit messages like this:

BREAKING-RELEASE: update to @company/my-cool-library 2.0

Using the problematic / in npm library names. And very often we forget that the slash in there breaks semantic-release.

@mikehardy
Copy link

mikehardy commented Dec 12, 2021

[EDIT] Never mind, my commit failed because angular is the preset, and angular both does not recognize ! nor does it recognize BREAKING CHANGES (with the S). I was used to conventional commits spec and angular was unexpectedly-to-me tighter

I altered my release config to use conventional commits and all is well. Apologies for the noise

previous comment contents based on flawed assumptions Wow, I think this is what just failed my run:

[2:05:07 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"
[2:05:07 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: feat(android, sdks)!: update to the latest v20 android admob sdk (#32)

* Add  onAdClicked()  and Event opened
* Add fullScreenContentCallback for rewarded ad
* Up compile sdk version for example
* Lint fixes
* Move setting test ids to ReactNativeGoogleAdsModule
* Remove smart banner
* Remove AdEvent left application

BREAKING CHANGES:
Please refer to upstream guides for suggestions on new usage. https://developers.google.com/admob/ios/migration and https://developers.google.com/admob/android/migration
- compileSdkVersion now 31, change your app android build.gradle to 31 if you have not already. Note that JDK11 is required for stable compilation on compileSdkVersion 31, JDK8 has internal compiler errors with SDK31
- onAdLeftApplication removed from the underlying SDK, use react-native built in AppState to determine app went to background
- Smart banner ads removed; use adaptive banner ads. Set height/width explicitly taking into account device size

The thing had a !, it had BREAKING CHANGES:, and still nothing launched. Quite a surprise. Does that mean URLs may not be in commit messages until this is fixed?

https://github.com/invertase/react-native-google-ads/runs/4498178356?check_suite_focus=true#step:5:43

I can't tell if it is this one or #231 that just bit me.

@kontza
Copy link

kontza commented Feb 23, 2023

Hi,

Just wanted to chime in to this one, that this problem bites me constantly. We use Bitbucket, and our pull requests follow this pattern Pull request #42: Feature/SMKY-1231 Working with responsiveness

That forward slash is 'poison' for picomatch (which micromatch uses under the covers). Both of those libraries seem to be designed for filesystem glob matching hence it makes sense that it would do some magic when forward slashes are included in strings.

Would be it be possible for commit-analyzer to drop this glob-approach and start to use regular expressions? That would be a big change, I know, but the result would be more predictable.

Or, can someone give a pointer what would be the least effort path to get semantic-release to trigger a patch-release for Bitbucket PRs (looking like the sample line above)?

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

No branches or pull requests

7 participants