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 multiple notification endpoints to existing webhook s… #108

Merged
merged 7 commits into from
Apr 26, 2017

Conversation

ryan-codingintrigue
Copy link
Contributor

This extends the existing webhook system to allow for posting to multiple endpoints based off the package name being published.

The current global webhook system remains untouched to allow for backwards compatibility - it will still run alongside any webhooks defined in a collection under the notify node.

It also adds regular expression support to the existing global webhook.

@juanpicado
Copy link
Member

@ryan-codingintrigue
Copy link
Contributor Author

Re-ran the Travis build and seems to have passed checks. Must have been a transient error

@juanpicado
Copy link
Member

@ryan-codingintrigue Could you help me to fix the conflicts? I think I screwed up your PR fixing another bug.

@ryan-codingintrigue
Copy link
Contributor Author

Thanks @juanpicado - I think that should be correct now. Apologies, I've never re-based a fork on Github before so this is quite new to me!

lib/notify.js Outdated
if (config.notify && config.notify.content) {
var handleNotify = function(metadata, notifyEntry) {
var regex
if(metadata.name && notifyEntry.packagePattern) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you standardize code styling? eg: if ( instead if(. The same below for for below. In part is my fault because eslint strict styling hasn't been worked out yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juanpicado Thanks a lot, sorry for the delay on this. Migrated these changes to ES6 syntax and fixed those code styling issues.

@juanpicado
Copy link
Member

thanks @ryan-codingintrigue 👍 again, I'll merge it today.

@juanpicado juanpicado merged commit 472e8e9 into verdaccio:master Apr 26, 2017
@juanpicado juanpicado added this to the 2.1.6 milestone May 12, 2017
@juanpicado
Copy link
Member

juanpicado commented Jul 1, 2017

I was doing unit testing for it and I noticed in Node without full ES6 support fails if you use packagePatternFlags. So I'll remove support for packagePatternFlags until Node 4 is dropped.

TypeError: Cannot supply flags when constructing one RegExp from another
    at new RegExp (native)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp

Starting with ECMAScript 6, new RegExp(/ab+c/, 'i') no longer throws a TypeError ("can't supply flags when constructing one RegExp from another") when the first argument is a RegExp and the second flags argument is present. A new RegExp from the arguments is created instead.

juanpicado added a commit that referenced this pull request Jul 1, 2017
@lock
Copy link

lock bot commented May 31, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018
@juanpicado
Copy link
Member

@ryan-codingintrigue node 4 was dropped long time ago, https://github.com/verdaccio/verdaccio/blob/master/src/lib/notify.js#L9
I think we can reenable this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants