-
Notifications
You must be signed in to change notification settings - Fork 369
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
Demux Github webhook events based on included regions #282
base: master
Are you sure you want to change the base?
Conversation
…lean up old unused code
@@ -0,0 +1 @@ | |||
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.5.4/apache-maven-3.5.4-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to make this a separate PR and use the latest version, 3.6.3 currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salmaanrizvi
I agree with @timja here. I'd rather the discuss the pros/cons of adding maven wrapper in another PR.
import java.nio.channels.*; | ||
import java.util.Properties; | ||
|
||
public class MavenWrapperDownloader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salmaanrizvi
This looks like an interesting and useful change, but it needs some changes.
- Merge with current master
- The maven wrapper addition needs to be a separate PR
- More comments either in the PR and or in the code for what the code is doing.
- Add tests
I've also started a gitter.im channel for this plugin. Happy to discuss over there as well.
Is this not basically JENKINS-58637 and handled already by https://github.com/jenkinsci/multibranch-build-strategy-extension-plugin? |
I'm not sure this is the best way to do this, but would love to know of any cases / pitfalls I may have not considered properly in this implementation.