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

Add support for #none# version bump option #67

Merged
merged 8 commits into from
Feb 9, 2017

Conversation

sandersky
Copy link
Contributor

@sandersky sandersky commented Feb 9, 2017

This project uses semver, please check the scope of this pr:

  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

resolves #49

CHANGELOG

  • Added support for new #none# scope to support pull requests that do not bump the version resolving #49 and #40 (with the use of tags: true in .travis.yml)

@codecov-io
Copy link

codecov-io commented Feb 9, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@ab26afb). Click here to learn what that means.

@@            Coverage Diff            @@
##             master      #67   +/-   ##
=========================================
  Coverage          ?   89.49%           
=========================================
  Files             ?       10           
  Lines             ?      419           
  Branches          ?       49           
=========================================
  Hits              ?      375           
  Misses            ?       44           
  Partials          ?        0

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 ab26afb...46c2e93. Read the comment docs.

@sandersky
Copy link
Contributor Author

@job13er I'm not sure if you have a way to truly test this before it is merged or not but the changes looked pretty straightforward to me.

@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

Have you given any thought to how to prevent the publish of packages which get the #none# directive? Not taking the time to figure out how that would work is what has kept me from implementing this thus far. I didn't want to support the option without understanding how it can be used.

@sandersky
Copy link
Contributor Author

@job13er valid point. It actually won't publish as npm won't let you publish the same version twice. However every time you leverage #none# you'll get a failed merge build which isn't ideal. I'll give this some more thought.

@sandersky sandersky changed the title Add support for #none# verson bump option DO NOT MERGE – Add support for #none# verson bump option Feb 9, 2017
@sandersky
Copy link
Contributor Author

@job13er I see Travis CI's npm deploy allows you to set tags: true to only deploy on tagged commits. I've updated this PR to not add a tag or append to the CHANGELOG when the scope is #none#. Unless I'm missing something I think this will handle it. We just need to make sure our repositories that rely on pr-bumper set tags: true in their .travis.yml.

@sandersky sandersky changed the title DO NOT MERGE – Add support for #none# verson bump option Add support for #none# verson bump option Feb 9, 2017
@sandersky
Copy link
Contributor Author

Here is the Travis documentation on the tags option btw https://docs.travis-ci.com/user/deployment/npm/#What-to-release

@sandersky
Copy link
Contributor Author

I've verified this change works in this pull request which uses my fork of pr-bumper. When using #none# it doesn't complain if the changelog section is missing and upon merging does not try to publish so I still get a green build (leverage the tags: true option in my Travis config).

screen shot 2017-02-08 at 5 27 15 pm

@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

Sweet! I knew there was a tags option, but I was thinking it was deciding what to build, not what to deploy. We don't wanna re-run the build on those tags, but we do want to publish. Excellent find.

@sandersky sandersky changed the title Add support for #none# verson bump option Add support for #none# version bump option Feb 9, 2017
@sandersky
Copy link
Contributor Author

Verified it again after my last commits ciena-blueplanet/bunsen-core#69

@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

👍

Approved with PullApprove

@job13er job13er merged commit 3119c9f into ciena-blueplanet:master Feb 9, 2017
@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

Oh no, I just realized something, shouldn't have merged this. Did you confirm that you can actually deploy when using something other than #none# with this. I have a bad suspicion that we're not building the tagged commit and so it'll never publish. B/c the commit being built isn't the pr-bumper tagged commit, it's the one previous. By setting the tags: true I think we may end up double-bumping:

PR Merge commit builds (not tagged, not deployed)
pr-bumper bump commit builds (is tagged, is deployed), possibly creates another pr-bumper bump commit.

We'll find out as the merge for this PR builds how it actually works. I think that either
a) It won't publish at all
or
b) it'll publish too many times

Hopefully I'm wrong

@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

So, looks like it was a) that is what happend:

screen shot 2017-02-09 at 4 48 05 am

@job13er
Copy link
Contributor

job13er commented Feb 9, 2017

So, because we have the actual bump as a before_deploy and deploy isn't happening except on a tag pr-bumper is never trying to bump :(

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.

[Discussion] Should there be a #none# directive?
3 participants