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 symbols to extension points #50

Merged
merged 4 commits into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@mezpahlan
Copy link

commented Jun 3, 2018

The PR allows easier pipeline syntax instead of referring to the General Build Step and targetting the plugin class you can now do something similar to:

hockeyApp applications: [[downloadAllowed: false, mandatory: true, notifyTeam: true, releaseNotesMethod: changelogReleaseNotes(), uploadMethod: appCreation(false)]], debugMode: false, failGracefully: false

We are still targeting Jenkins 1.x hence the need to declare structs explicitly (if I understand correctly).

@mezpahlan

This comment has been minimized.

Copy link
Author

commented Jun 5, 2018

Hi @jenkinsci/code-reviewers could I have some quick comments on this please. I'm looking to make the pipeline syntax more friendly for users. The names of the symbols may change but I'm interested in comments about this approach and what could be improved.

@oleg-nenashev
Copy link
Member

left a comment

looks good to me

@mezpahlan mezpahlan force-pushed the mezpahlan:feature/support-pipeline-syntax branch from f3640f1 to 6df3a99 Jun 9, 2018

mezpahlan added some commits Jun 9, 2018

Remove annotation-indexer
Seems to cause problems with this included and no problems without.
Strange..... Can't see where this is used so removing for now.
Bump structs to 1.13
Note: 1.14+ requires that we bump the minimum supported Jenkins version
for this plugin.

Remove structs test dependency

This is taken care of with the compile dependency of the same plugin.
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 10, 2018

Looks reasonable.

We are still targeting Jenkins 1.x

FYI: 94% of users running version 1.2.2 (released in early 2017) of your plugin, which are ~80% of all plugin installations, are on Jenkins 2.32.1 or newer based on anonymous usage stats ( http://stats.jenkins.io/pluginversions/hockeyapp.html ; which seems to include wrong versions, perhaps private builds, but only one install each, so insignificant)

Users who don't update Jenkins core don't update plugins either.

@mezpahlan

This comment has been minimized.

Copy link
Author

commented Jun 10, 2018

Thanks @daniel-beck. I want to do the following in the long run:

  1. Get up to speed fixing various long outstanding bugs.
  2. Provide some level of pipeline support.
  3. Refactor the code base to be more testable.
  4. Only support Jenkins 2.x.

So it is on the plan but perhaps I should think again at the order I do these given the stats.

@mezpahlan mezpahlan merged commit a574cdd into jenkinsci:master Jun 11, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@mezpahlan mezpahlan deleted the mezpahlan:feature/support-pipeline-syntax branch Jun 11, 2018

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

@oleg-nenashev Do you know what the conflict potential for symbols is when multiple plugins define the same symbols? Especially after the last commit in this PR, this could be a potential problem.

@mezpahlan

This comment has been minimized.

Copy link
Author

commented Jun 11, 2018

@mezpahlan

This comment has been minimized.

Copy link
Author

commented Jun 12, 2018

To add some context to this. The top level symbol name has not changed between the start and end of the PR. Only an internal descriptor for the releaseNotesMethod. To illustrate:

At start of PR:

hockeyApp applications: [[apiToken: 'secret-token', filePath: 'sample.apk', releaseNotesMethod: manualReleaseNotes(isMarkdown: true, releaseNotes: 'Bug fixes and minor updates'), uploadMethod: versionCreation('app-token')]], debugMode: false, failGracefully: false

At end of PR:

hockeyApp applications: [[apiToken: 'secret-token', filePath: 'sample.apk', releaseNotesMethod: manual(isMarkdown: true, releaseNotes: 'Bug fixes and minor updates'), uploadMethod: versionCreation('appt-token')]], debugMode: false, failGracefully: false

The top level symbol remains as hockeyApp. Note the small difference in the symbol for the class we send into releaseNotesMethod.

  • manualRelaseNotes -> manual.
  • changelogReleaseNotes -> changelog
  • fileReleaseNotes -> file
  • noReleaseNotes -> none

The last commit was purely cosmetic to remove the (in my opinion) redundant ReleaseNotes suffix on the symbol.

Anyways, let me know if that makes a difference. Happy to revert it if it will cause collisions with other plugins.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Perhaps @ndeloof knows more about how symbols work and whether these names are safe, given his recent work.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

@mezpahlan @daniel-beck

Do you know what the conflict potential for symbols is when multiple plugins define the same symbols? Especially after the last commit in this PR, this could be a potential problem

@Symbol engine is designed to support symbol overlaps. As long as there is no colliding symbols for a same extension point, it should be fine

@mezpahlan

This comment has been minimized.

Copy link
Author

commented Jun 12, 2018

Thanks @oleg-nenashev. How would I test for that?

As long as there is no colliding symbols for a same extension point, it should be fine

Would it be obvious and give me a compilation error or should I run some more robust tests?

@ndeloof

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

unfortunately no, there's no way for you to discover a collision. This would require we know about ALL jenkins plugins (including proprietary ones). But this is something we could add in CI in a near future as we generate docs based on community plugins.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Yes, it would be a valid and helpful use-case for the Extension Points Documentation generator. CC @daniel-beck

@@ -32,6 +32,11 @@
</licenses>

<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>

This comment has been minimized.

Copy link
@ndeloof

ndeloof Jun 12, 2018

Member

structs-annotations would be enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.