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-37054] Add symbol annotation #30

Merged
merged 2 commits into from
Nov 10, 2016

Conversation

alexsomai
Copy link
Member

Related to issue [JENKINS-37054]

Summary of this pull request:

  • Add @Symbol annotation

With the @Symbol annotation, the Pipeline code can drop the $class syntax

Examples:

  1. wsCleanup()
    instead of
    step([$class: 'WsCleanup'])
  2. wsCleanup cleanWhenFailure: false
    instead of
    step ([$class: 'WsCleanup', cleanWhenFailure: false])
  3. wsCleanup patterns: [[pattern: 'bar.*', type: 'INCLUDE']]
    instead of
    step([$class: 'WsCleanup', patterns: [[pattern: 'bar.*', type: 'INCLUDE']]])

CC @olivergondza @oleg-nenashev @martinda

@@ -16,7 +16,10 @@
<properties>
<jenkins.version>1.609</jenkins.version>
<java.level>6</java.level>
<workflow.version>1.4.3</workflow.version>
<workflow-job.version>1.4.3</workflow-job.version>
<workflow-cps.version>1.4.3</workflow-cps.version>
Copy link
Member

Choose a reason for hiding this comment

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

Is it enough for testing of the feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, minimum workflow-cps 2.10 is required.
I have added them as separate properties to be able to test it like this:
https://github.com/jenkinsci/ws-cleanup-plugin/pull/30/files#diff-99947b5d4e31828f2c311a58853445a6R340

mvn clean install -Djenkins.version=1.642.1 -Djava.level=7 -Dworkflow-job.version=2.4 -Dworkflow-basic-steps.version=2.1 -Dworkflow-cps.version=2.10 -Dworkflow-durable-task-step.version=2.4

Copy link
Member

Choose a reason for hiding this comment

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

Why does the plugin not require the versions it needs?

Copy link
Member Author

Choose a reason for hiding this comment

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

workflow-cps version 2.10 works only with minimum Jenkins core version 1.642.1. This means that I need to bump the core version to 1.642.1 and java level to 7, which I don't think is a good idea, yet.

Copy link
Member

Choose a reason for hiding this comment

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

@alexsomai, Let's do that not to complicate things this way. If specific properties are needed to get this fully tested, it is almost as good as untested since release or ci would not execute that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@olivergondza I've bumped the Jenkins core version and other dependencies to minimum required versions to fully test the symbol annotation

@martinda
Copy link

martinda commented Sep 3, 2016

If the name is wsCleanup then people may associated it too strongly to the ws step? Perhaps cleanupWorkspace is better. No strong opinion.

@oleg-nenashev
Copy link
Member

@olivergondza Which name would you prefer? :)

Actually we could use multiple names

@olivergondza
Copy link
Member

I am fine with wsCleanup as I do not think the association with ws step is harmful.

@alexsomai
Copy link
Member Author

@olivergondza I've been thinking about it, and maybe it would be better if the step name begins with the verb? so cleanupWs, or cleanupWorkspace (as @oleg-nenashev and @martinda suggested).
Or, I can go with both of them, and let the user decide which he wants to use.

@martinda
Copy link

martinda commented Sep 5, 2016

@oleg-nenashev @alexsomai @olivergondza Sorry I should not have started this name debate, it does not feel productive. IMO if every plugin provided multiple names for the same step, I think the DSL would be harder to learn. I would prefer a single name.

For the RunSelector plugin step, the step name selectRun was selected over the other proposed step name runSelector, when jglick recommended using a verb, however, after looking up existing step names, I do not have a strong opinion, whatever the decision is, I will accept it.

@olivergondza
Copy link
Member

I lean towards having one alternative only as well. Sticking to the verb form, any objections to cleanWs?

@alexsomai
Copy link
Member Author

+1 for cleanWs

@alexsomai
Copy link
Member Author

I've changed the symbol value to cleanWs. I've made the change with a force push

@oleg-nenashev
Copy link
Member

👍

@martinda
Copy link

I thought I had already +1, but here it is.

@alexsomai
Copy link
Member Author

@olivergondza do you think is there anything else that I should do on this PR?

@olivergondza olivergondza merged commit 9e1004b into jenkinsci:master Nov 10, 2016
@adamvoss
Copy link

adamvoss commented May 3, 2018

The description for this on Jenkins reads "Delete workspace when build is done". Does that mean that I can put this command anywhere in my pipeline and it will cause the deletion to happen when everything else is done?

@alexsomai
Copy link
Member Author

@adamvoss Yes, it should delete your current workspace when the build has completed successfully. But you also have other options, as in the documentation https://jenkins.io/doc/pipeline/steps/ws-cleanup/#workspace-cleanup-plugin

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

Successfully merging this pull request may close these issues.

5 participants