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

Simplify AnyScript #532

Merged
merged 1 commit into from
May 26, 2022
Merged

Simplify AnyScript #532

merged 1 commit into from
May 26, 2022

Conversation

jglick
Copy link
Member

@jglick jglick commented May 25, 2022

Simplifying a rather roundabout system to just be like

return {
script.node(describable?.label) {
CheckoutScript.doCheckout(script, describable, describable.customWorkspace, body).call()
}
}
except without the label and customWorkspace fields. The weird logic seems to have been introduced in #94, at which time perhaps it made sense but it seems CheckoutScript when introduced in #123 was not also used here.

Comment on lines -39 to -40
Label l = (Label) Label.DescriptorImpl.instanceForName("label", [label: null])
l.copyFlags(describable)
Copy link
Member Author

@jglick jglick May 25, 2022

Choose a reason for hiding this comment

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

Note two methods which are no longer used in non-test code in this plugin but still used in https://github.com/jenkinsci/docker-workflow-plugin/blob/c7dedefcf68f6ce1320c8aaaeb831628ea21a086/src/main/java/org/jenkinsci/plugins/docker/workflow/declarative/DeclarativeDockerUtils.java#L91-L92 which could probably also be simplified along similar lines though I am not inclined to spend time on that plugin.

The analogous code in kubernetes is already straightforward: https://github.com/jenkinsci/kubernetes-plugin/blob/44fee1844a33a5946bf9f721edb9da64b7b7b3da/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesDeclarativeAgentScript.groovy#L52-L53

@jglick
Copy link
Member Author

jglick commented May 25, 2022

(Desired for #533.)

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

as far as I understand this code, I see nothing obviously wrong

@jglick jglick merged commit f9572ff into jenkinsci:master May 26, 2022
@jglick jglick deleted the AnyScript branch May 26, 2022 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants