-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
JENKINS-56284 Make compute changelog extensible #813
base: master
Are you sure you want to change the base?
Conversation
@MarkEWaite Previous context #682 |
Extensions are unfamiliar to me. I've requested review from @rsandell and @fcojfernandez since they likely have more experience with using the extensions API than I do. |
@gmshake could you elaborate a bit your proposal? You've created some classes named as "Extension" but they are not annotated. What's the expected behaviour? How is it intended that the downstream plugins work with them? because I'm getting a bit confused. |
@fcojfernandez For the naming, I’m not sure which one is more appropriate, extension vs strategy. |
@gmshake it's not a matter of the name per se. What is unclear for me is how this new class will be consume? In your JIRA ticket you're mentioning |
* FIXME JavaDoc | ||
* @author Zhenlei Huang | ||
*/ | ||
public abstract class GitSCMChangelogExtension extends FakeGitSCMExtension { |
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.
For example, this is something I don't understand. Why extending FakeGitSCMExtension
instead of GitSCMExtension
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.
I’m not sure which one is better. If I remember correctly, extending GitSCMExtension is also OK.
@fcojfernandez Sorry for late response. I have a local fork of bitbucket-branch-source-plugin that consume this plugin. But currently I’m not able to push it to github to demonstrate the usage because of regulations caused by 2019-nCoV. I’ll push when I’m able to do. |
No worries. Just ping me again when you can do it. At a first glance I don't see anything that shocks me, but I'd like to be to test it and see how this extension is consumed by other plugins Just a couple of things for you to address once you have availability:
|
1916bf9
to
0b103ee
Compare
I'm back :) Rebase on latest master. Downstream usage :
|
JENKINS-56284 - Make compute changelog extensible
Checklist
Types of changes
What types of changes does your code introduce? Put an
x
in the boxes that apply. Delete the items in the list that do not applyFurther comments
If this is a relatively large or complex change, start the discussion by explaining why you chose the solution you did and what alternatives you considered.