-
Notifications
You must be signed in to change notification settings - Fork 54
Add symbols to extension points #50
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
Conversation
|
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
left a comment
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.
looks good to me
f3640f1 to
6df3a99
Compare
Seems to cause problems with this included and no problems without. Strange..... Can't see where this is used so removing for now.
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.
|
Looks reasonable.
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. |
|
Thanks @daniel-beck. I want to do the following in the long run:
So it is on the plan but perhaps I should think again at the order I do these given the stats. |
|
@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. |
|
Apologies, I wasn't aware of the conflict potential in symbol names before
I refactored.
Let me know what the risk is and I will do another PR based on the
decision, if needed. I won't release this until someone has confirmed it is
safe.
|
|
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: falseAt 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: falseThe top level symbol remains as
The last commit was purely cosmetic to remove the (in my opinion) redundant Anyways, let me know if that makes a difference. Happy to revert it if it will cause collisions with other plugins. |
|
Perhaps @ndeloof knows more about how symbols work and whether these names are safe, given his recent work. |
|
|
Thanks @oleg-nenashev. How would I test for that?
Would it be obvious and give me a compilation error or should I run some more robust tests? |
|
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. |
|
Yes, it would be a valid and helpful use-case for the Extension Points Documentation generator. CC @daniel-beck |
| <dependencies> | ||
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>structs</artifactId> |
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.
structs-annotations would be enough
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:
We are still targeting Jenkins 1.x hence the need to declare structs explicitly (if I understand correctly).