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 flag to fail publishing if version conflict in NPM #44

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@jeanml

jeanml commented Nov 30, 2018

Builds can silently fail if plugin tries to publish a package version that already exists in the target registry. To avoid this, this pull request adds an additional flag that forces the plugin to fail if there is a version conflict. Defaults to existing behavior if not specified.

Usage:

docker run --rm \
  -e NPM_USERNAME=drone \
  -e NPM_PASSWORD=password \
  -e NPM_EMAIL=drone@drone.io \
  -e PLUGIN_FAIL_ON_VERSION_CONFLICT=true \
  -v $(pwd):$(pwd) \
  -w $(pwd) \
  plugins/npm
@tboerger

This comment has been minimized.

Member

tboerger commented Nov 30, 2018

Maybe name the attribute conflict_fail?

@jeanml

This comment has been minimized.

jeanml commented Nov 30, 2018

I was actually thinking that this really should be the default behavior and suggesting renaming to ignore_conflict. By default that would be false and the step would fail if there is a version conflict.

I am fully aware that this is a breaking change but if it breaks anyone's build I'd like to think that it's a good thing. Any thoughts?

@donny-dont

This comment has been minimized.

Contributor

donny-dont commented Nov 30, 2018

@jeanml let me give some context on why it was done this way.

The NPM plugin was created over 3 years ago, which is around the Drone 0.3 release, so its one of the oldest plugins for Drone. At the time I don't believe that when you enabled Drone on a repo that it would actually build tags by default. So it assumed that someone using it wouldn't know that they needed to turn on that within the settings. I'm also not sure how robust the conditionals were at that time.

This was a decision made on usability.

I don't know the state of 1.0 with regards to what it builds. I don't see those options where you can turn off and on tags in cloud.drone.io for a repo I enabled. So maybe now it just turns everything on by default.

Now I agree with you that in an ideal world people should just have a when clause looking for a tag and only run the plugin on those events. However looking at Drone Plugins the NPM plugin is the 5th most downloaded plugin (the stuff with drone- prefixed are pre 0.5 plugins) so its really hard to introduce that sort of breaking change.

There are some breaking changes I'd like to do to the yaml for s3-cache but that one as well is used substantially. I'm not sure what exactly one would want to do here.

@tboerger @bradrydzewski is there anywhere to discuss breaking changes like this in a plugin?

@tboerger

This comment has been minimized.

Member

tboerger commented Dec 1, 2018

Discussions are best on discourse. I personally wouldn't add that breaking change.

@jeanml

This comment has been minimized.

jeanml commented Dec 3, 2018

@donny-dont Not sure I am totally following you on your explanation relating to tags. What I really want to avoid here is an NPM publish Drone step silently failing to publish (in our case, to an internal Artifactory NPM registry) if there is a version conflict.

In any case, I totally get that given the popularity of the plugin, introducing a potentially (it only fails if you do actually have a version conflict) breaking change is perhaps not desirable.

The code change in the pull request is non-breaking and requires opting-in to fail on version conflict. Happy to go with that option as well! I don't mind renaming the plugin property to conflict_fail as suggested by @tboerger or fail_on_version_conflict or leaving it as is (fail_if_conflict).

@tboerger

This comment has been minimized.

Member

tboerger commented Dec 3, 2018

I'm for this current non-breaking behavior. Now it's just a matter of taste how this attribute got to be named :)

@jeanml

This comment has been minimized.

jeanml commented Dec 4, 2018

I have updated the pull request to use fail_on_version_conflict. Ready to merge as far as I'm concerned if you are happy with the change.

@donny-dont

This comment has been minimized.

Contributor

donny-dont commented Dec 4, 2018

@jeanml the correct way to do things would probably be.

pipeline:
  ...

  publish:
    image: plugins/npm
    ...
    when:
      event: tag

The way things are done now is without the when tag because as I mentioned tags were not on by default when you enabled a repo. You had to go into the settings and enable them explicitly.

@tboerger

I don't have to like all namings 😂

@tboerger tboerger merged commit 95724a7 into drone-plugins:master Dec 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment