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 missing @Symbol for agent protocols #2960

Merged
merged 1 commit into from
Aug 4, 2017
Merged

add missing @Symbol for agent protocols #2960

merged 1 commit into from
Aug 4, 2017

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Aug 2, 2017

I just noticed JNLP3/4 and Ping agent protocols don't have @Symbol annotation, while JNLP1/2 have on

Changelog Entry

@ndeloof ndeloof requested a review from stephenc August 2, 2017 08:35
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐝 and CC @reviewbybees

@ndeloof do you consider it as a defect, which needs backporting to 2.60.3 or to 2.whatever.1? If yes, please create a JIRA ticket

@@ -307,6 +308,7 @@ private void error(PrintWriter out, String msg) throws IOException {
* @since 1.653
*/
@Extension
@Symbol("ping")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt there is a use-case for it, but OK

@oleg-nenashev
Copy link
Member

@ndeloof It would be also great if you start following the PR template as @daniel-beck suggested in your previous pull requests. It saves time for the PR integration.

@ndeloof
Copy link
Contributor Author

ndeloof commented Aug 2, 2017

definitively not a defect to be backported, I don't know where such @symbol is used for now, just noticed it was not used homogeneously, so this PR.

@oleg-nenashev
Copy link
Member

Added the changelog entry

@daniel-beck
Copy link
Member

daniel-beck commented Aug 2, 2017

What's the point of this, given the state https://github.com/jenkinsci/system-config-dsl-plugin is in? Does anyone care about this? Job DSL perhaps?

Updated to add: Missed previous comments explaining this is only for consistency. Sure. Whatever.

Not worth a changelog entry as it affects nobody, similar to Javadoc updates.

@oleg-nenashev
Copy link
Member

@daniel-beck JNLP/JNLP2 are already annotated, hence it is just a consistency change. It may affect configuration-as-code use-cases, so I vote for changelogging it as RFE

@ndeloof
Copy link
Contributor Author

ndeloof commented Aug 2, 2017

@daniel-beck this is not for system-config-dsl-plugin but some research project I'm working on, I just noticed those components where missing @symbol while others do have one, wanted to make this minor improvement for homogeneous API, nothing much.

@daniel-beck
Copy link
Member

@oleg-nenashev

It may affect configuration-as-code use-cases

system-config-dsl isn't anywhere near done, does anyone actually use that? And nothing else uses symbols I'm aware of. Unless you know another (public) project, it's too irrelevant to mention.

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 2, 2017
@oleg-nenashev
Copy link
Member

@reviewbybees done

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Aug 2, 2017
@ghost
Copy link

ghost commented Aug 2, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev oleg-nenashev merged commit 9d090be into jenkinsci:master Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants