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

JENKINS-56284 Make compute changelog extensible #813

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gmshake
Copy link

@gmshake gmshake commented Jan 13, 2020

JENKINS-56284 - Make compute changelog extensible

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

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 apply

  • Dependency or infrastructure update
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Further 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.

@gmshake
Copy link
Author

gmshake commented Jan 13, 2020

@MarkEWaite Previous context #682

@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Jan 17, 2020
@MarkEWaite
Copy link
Contributor

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.

@fcojfernandez
Copy link
Contributor

@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.

@gmshake
Copy link
Author

gmshake commented Feb 3, 2020

@fcojfernandez For the naming, I’m not sure which one is more appropriate, extension vs strategy.
The current implementation of computing changelog is somewhat rigid and this plugin provides fixed number of strategies. I intent to make it more flexible thus downstream projects can provide their own strategies.

@fcojfernandez
Copy link
Contributor

@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 github-branch-source-plugin. How this plugin would make use of GitSCMChangelogExtension? I need to understand better this before reviewing the PR

* FIXME JavaDoc
* @author Zhenlei Huang
*/
public abstract class GitSCMChangelogExtension extends FakeGitSCMExtension {
Copy link
Contributor

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

Copy link
Author

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.

@MarkEWaite MarkEWaite removed their request for review February 8, 2020 21:01
@gmshake
Copy link
Author

gmshake commented Feb 12, 2020

@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.

@fcojfernandez
Copy link
Contributor

@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:

  • I'm missing some tests
  • There are merge conflicts

@gmshake
Copy link
Author

gmshake commented Jun 14, 2020

I'm back :) Rebase on latest master.

Downstream usage :

  1. bitbucket-branch-source-plugin: https://github.com/gmshake/bitbucket-branch-source-plugin/pull/1/files
  2. github-branch-source-plugin: https://github.com/gmshake/github-branch-source-plugin/pull/1/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants