[JENKINS-26097] Adjust label expression auto-completion and validation #4774

Zastai · 2020-06-06T16:21:38Z

See JENKINS-26097.

This moves label expression auto-completion and validation from AbstractProject to LabelExpression.

Auto-Completion:

AbstractProject.AutoCompleteSeeder was moved to LabelExpression
- test class correspondingly moved to the relevant package
added static LabelExpression.autoComplete() method
- does what the non-static AbstractProject.doAutoCompleteLabel() used to do; that now calls the new static method

Validation:

added LabelExpression.LabelValidator, which is like AbstractProject.LabelValidator except it takes a Job instead of an AbstractProject
- AbstractProject.LabelValidator is now deprecated
added static LabelExpression.validate(String, Job) method, which replaces AbstractProject.validateLabelExpression(String, AbstractProject)
- the latter is marked as deprecated and forwards to the former
- any warnings/errors reported by validators are now aggregated (previously only the first one was shown)
LabelExpressionTest.formValidation() uses the new static method
associated messages were moved from hudson.model.Messages.AbstractProject_xxx() to
hudson.model.Messages.LabelExpression_xxx()).

Proposed changelog entries

JENKINS-26097: hudson.model.AbstractProject.LabelValidator is now deprecated in favour of jenkins.model.labels.LabelValidator
- but the old version also has a new checkItem() method to allow them to apply validation to non-Project items too
JENKINS-26097: hudson.model.AbstractProject.DescriptorImpl.validateLabelExpression() is now deprecated in favour of LabelExpression.validate() (which takes any kind of Item object instead of only AbstractProjects)
- this will now aggregate all warnings and errors reported by LabelValidators (old and new)
JENKINS-26097: auto-completion for labels is now available via LabelExpression.autoComplete()

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Reference implementations:

Desired reviewers

@jglick

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

This moves label expression auto-completion and validation from AbstractProject to LabelExpression. Auto-Completion: - AbstractProject.AutoCompleteSeerder was moved to LabelExpression - test class correspondingly moved to the relevant package - added static LabelExpression.autoComplete() method - does what the non-static AbstractProject.doAutoCompleteLabel() used to do; that now calls the new static method Validation: - added LabelExpression.LabelValidator, which is like AbstractProject.LabelValidator except it takes a Job instead of an AbstractProject - AbstractProject.LabelValidator is now deprecated - added static LabelExpression.validate(String, Job) method, which replaces AbstractProject.validateLabelExpression(String, AbstractProject) - the latter is marked as deprecated and forwards to the former - LabelExpressionTest.formValidation() uses the new static method

…llers.

jglick

Looks reasonable from a quick glance. (Blocks of code moved around makes it hard to read the diff.) Would make sense to consume this from workflow-durable-task-step-plugin so that snippet generator on the node step can offer the same conveniences that you get in a freestyle project’s config screen.

core/src/main/java/hudson/model/labels/LabelExpression.java

In addition, the new LabelValidators are now applied to AbstractProjects too.

jglick

Would still recommend demonstrating usage in workflow-durable-task-step-plugin.

Zastai · 2020-06-11T15:42:56Z

I merged current master so the build produces an incremental; will look at creating a PR on workflow-durable-task-step-plugin using that incremental.

Zastai · 2020-06-11T16:32:34Z

Looking at workflow-durable-task-step-plugin I am now confused. I always thought that label property was just informational text and was in no way related to the label expressions you have elsewhere (like tool installers and projects). So I'm not sure it makes sense adding the label expression validation and autocompletion there.

If there was a snippet generator for the agent part of a declarative pipeline (or the node for a scripted one), then sure, it makes sense there.

Zastai · 2020-06-11T16:56:15Z

I had missed ExecutorStep; that does have an applicable field.

jglick · 2020-06-11T18:18:38Z

Yes on ExecutorStep, not DurableTaskStep, sorry if I was unclear. And yes there could be a similar fix for https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/d9f665d27c269f8294e3897f54f1f7d1f3d6a8e5/pipeline-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/Label.java#L74-L80 as well, though I seem to recall Declarative snippet generation being more generally broken (maybe false memory).

jglick · 2020-06-11T18:19:25Z

https://github.com/jenkinsci/workflow-durable-task-step-plugin/blob/cc78e242dac43e333be8969952721c2381827f6b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStep.java#L98-L114 specifically

Zastai · 2020-06-11T18:29:53Z

I have changes in progress for that and the one other use of the old validation method (other than my own plugin), in matrix-project-plugin (although that is still based on 2.60.3 so I don’t see that getting merged).

Zastai · 2020-06-12T11:31:24Z

Dropped my attempts to change matrix-project-plugin; it would first need to be updated to relatively recent dependency versions.

jglick · 2020-06-12T17:15:55Z

Dropped my attempts to change matrix-project-plugin; it would first need to be updated to relatively recent dependency versions.

Check jenkinsci/matrix-project-plugin#62

Zastai · 2020-06-13T21:08:56Z

OK, three plugins now have reference implementations of the changed API; main PR comment updated.

Main open item from my end is whether or not I should move the i18n messages (to Messages.LabelExpression_xxx from Messages.AbstractProject_xxx). Messages is no-external-use, so there shouldn't be outside code referencing them, but I want to be sure it's OK.

oleg-nenashev

It looks good in principle, hence I am approving it.
At the same time it could be possible to enhance how API is provided and to simplify adoption of the API in the code

core/src/main/java/hudson/model/labels/LabelExpression.java

core/src/main/java/hudson/model/AbstractProject.java

…leteSeeder. This also adds missing nullability annotations.

These are now under LabelExpression.xxx instead of AbstractProject.xxx. For the translations, properties were either renamed in-place or moved, depending on whether the file seemed to be sorted alphabetically by property name.

Zastai · 2020-06-15T18:57:07Z

Suggestions applied; messages moved (no message changes, except that a stray trailing \ was dropped in the Japanese file).

core/src/main/java/hudson/model/labels/LabelAutoCompleteSeeder.java

core/src/main/java/hudson/model/AbstractProject.java

By default, this calls the regular check() if the item is an AbstractProject; otherwise, it returns OK. This allows a plugin to implement an override for that method in order to have its validator applied to Jobs too, without needing to bump their Jenkins dependency in order to get the new LabelValidator. As a result, LabelExpression.validate() now always uses the "old" LabelValidators, calling this new method.

core/src/main/java/hudson/model/labels/LabelAutoCompleteSeeder.java

core/src/main/java/hudson/model/labels/LabelExpression.java

Moved the test class over as well. This also makes the constructor and getSeeds() method of LabelAutoCompleteSeeder public.

oleg-nenashev

Looks good to me after the patches, thanks a lot @Zastai !

jglick

Requesting API surface reduction. Otherwise looks good.

core/src/main/java/jenkins/model/labels/LabelValidator.java

core/src/main/resources/hudson/model/Messages.properties

core/src/main/java/jenkins/model/labels/LabelAutoCompleteSeeder.java

This avoids referring to a job (because the label validation can be applied completely outside of any job context, like in tool installers) or an assignment. Also aligned terms ("is serviced by" -> "matches").

This still discards all OKs, but instead of stopping at and returning the first warning/error reported, all warnings/errors are returned as an aggregated result. The JavaDoc for the relevant messages was extended to describe this behaviour.

jglick · 2020-06-24T17:27:58Z

core/src/main/java/hudson/model/labels/LabelExpression.java

-                if (!FormValidation.Kind.OK.equals(result.kind)) {
-                    return result;
+                if (FormValidation.Kind.OK.equals(result.kind)) {
+                    continue;


BTW I think if

jenkins/core/src/main/java/hudson/util/FormValidation.java

Lines 227 to 238 in 339f9b9

final StringBuilder sb = new StringBuilder("<ul style='list-style-type: none; padding-left: 0; margin: 0'>");

FormValidation.Kind worst = Kind.OK;

for (FormValidation validation: validations) {

sb.append("<li>").append(validation.renderHtml()).append("</li>");

if (validation.kind.ordinal() > worst.ordinal()) {

worst = validation.kind;

}

}

sb.append("</ul>");

return respond(worst, sb.toString());

were refined slightly, to ignore all occurrences of FormValidation.OK in its input, then the API and this impl could be simplified a bit while actually handling ok(String) from validators. Not necessary in this PR, just something I noticed while looking at aggregate.

Yes I looked there too. But I'm not sure it would help to get bulleted list entries for "Label is valid" (assuming a validator might put that in the OK text) among the errors/warnings.

Right—aggregate would need to be made smarter, perhaps:

if no arguments, or all FormValidation.OK, return FormValidation.OK

else consider all non-FormValidation.OK arguments with the worst status among the bunch, and

if only one, return that as is

else return a bulleted list of those messages

oleg-nenashev

Looks good to me, thanks for the adjustments in the pull request!

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

Zastai marked this pull request as draft June 6, 2020 16:21

Zastai added 3 commits June 6, 2020 19:11

Add auto-completion and validation for the label fields on tool insta…

9cbd50a

…llers.

Merge branch 'master' into JENKINS-26097

ab7fa7d

Use TODO, not FIXME for @SInCE.

14d1efb

Zastai marked this pull request as ready for review June 8, 2020 22:47

oleg-nenashev self-requested a review June 9, 2020 16:04

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jun 9, 2020

timja requested review from a team and jglick June 9, 2020 19:16

timja added developer Changes which impact plugin developers plugin-api-changes Changes the API of Jenkins available for use in plugins. and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jun 9, 2020

jglick reviewed Jun 9, 2020

View reviewed changes

core/src/main/java/hudson/model/labels/LabelExpression.java Outdated Show resolved Hide resolved

core/src/main/java/hudson/model/labels/LabelExpression.java Outdated Show resolved Hide resolved

core/src/main/java/hudson/model/labels/LabelExpression.java Outdated Show resolved Hide resolved

Zastai added 3 commits June 10, 2020 00:27

Use Item as validation context for the new API (instead of Job).

bb89a72

In addition, the new LabelValidators are now applied to AbstractProjects too.

Make LabelExpression.LabelValidator an interface.

3fb2b66

Fix a JavaDoc reference.

5519088

jglick approved these changes Jun 10, 2020

View reviewed changes

Merge branch 'master' into JENKINS-26097

0428cd8

Zastai mentioned this pull request Jun 12, 2020

[JENKINS-26097] Adjust label validation and auto-completion jenkinsci/workflow-durable-task-step-plugin#136

Merged

daniel-beck self-requested a review June 14, 2020 10:10

oleg-nenashev approved these changes Jun 15, 2020

View reviewed changes

Zastai changed the title ~~JENKINS-26097: Adjust label expression auto-completion and validation.~~ [JENKINS-26097] Adjust label expression auto-completion and validation. Jun 15, 2020

Zastai added 3 commits June 15, 2020 20:12

Move LabelValidator to the top level.

9a65888

Move LabelExpression.AutoCompleteSeeder to top level as LabelAutoComp…

7a3d722

…leteSeeder. This also adds missing nullability annotations.

Moved the i18n messages.

863eae5

These are now under LabelExpression.xxx instead of AbstractProject.xxx. For the translations, properties were either renamed in-place or moved, depending on whether the file seemed to be sorted alphabetically by property name.

oleg-nenashev reviewed Jun 15, 2020

View reviewed changes

core/src/main/java/hudson/model/labels/LabelAutoCompleteSeeder.java Show resolved Hide resolved

core/src/main/java/hudson/model/AbstractProject.java Show resolved Hide resolved

Zastai added 3 commits June 15, 2020 22:06

Add JavaDoc.

f0621c2

Merge branch 'master' into JENKINS-26097

d542a76

jglick reviewed Jun 16, 2020

View reviewed changes

core/src/main/java/hudson/model/labels/LabelAutoCompleteSeeder.java Outdated Show resolved Hide resolved

core/src/main/java/hudson/model/labels/LabelExpression.java Outdated Show resolved Hide resolved

Move new classes to jenkins.model.labels.

df6c373

Moved the test class over as well. This also makes the constructor and getSeeds() method of LabelAutoCompleteSeeder public.

oleg-nenashev approved these changes Jun 22, 2020

View reviewed changes

oleg-nenashev requested review from a team and jglick June 22, 2020 07:15

jglick requested changes Jun 23, 2020

View reviewed changes

core/src/main/java/jenkins/model/labels/LabelValidator.java Show resolved Hide resolved

core/src/main/resources/hudson/model/Messages.properties Outdated Show resolved Hide resolved

core/src/main/java/jenkins/model/labels/LabelAutoCompleteSeeder.java Outdated Show resolved Hide resolved

Zastai added 4 commits June 23, 2020 23:14

Merge branch 'master' into JENKINS-26097

0712143

Tweak messages.

1a0aea1

This avoids referring to a job (because the label validation can be applied completely outside of any job context, like in tool installers) or an assignment. Also aligned terms ("is serviced by" -> "matches").

Aggregate label validation problems.

6cfba40

This still discards all OKs, but instead of stopping at and returning the first warning/error reported, all warnings/errors are returned as an aggregated result. The JavaDoc for the relevant messages was extended to describe this behaviour.

Flag LabelAutoCompleteSeeder as NoExternalUse.

a94cb38

oleg-nenashev requested review from jglick and oleg-nenashev June 24, 2020 16:57

jglick approved these changes Jun 24, 2020

View reviewed changes

oleg-nenashev approved these changes Jun 25, 2020

View reviewed changes

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 Jun 25, 2020

timja added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Jun 29, 2020

timja changed the title ~~[JENKINS-26097] Adjust label expression auto-completion and validation.~~ [JENKINS-26097] Adjust label expression auto-completion and validation Jun 29, 2020

timja merged commit a49f580 into jenkinsci:master Jun 29, 2020

Zastai deleted the JENKINS-26097 branch June 29, 2020 07:31

daniel-beck mentioned this pull request Jul 27, 2020

Update at-since Javadoc #4877

Merged

6 tasks

	final StringBuilder sb = new StringBuilder("<ul style='list-style-type: none; padding-left: 0; margin: 0'>");
	FormValidation.Kind worst = Kind.OK;
	for (FormValidation validation: validations) {
	sb.append("<li>").append(validation.renderHtml()).append("</li>");

	if (validation.kind.ordinal() > worst.ordinal()) {
	worst = validation.kind;
	}
	}
	sb.append("</ul>");

	return respond(worst, sb.toString());

Uh oh!

[JENKINS-26097] Adjust label expression auto-completion and validation #4774

[JENKINS-26097] Adjust label expression auto-completion and validation #4774

Uh oh!

Conversation

Zastai commented Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Zastai commented Jun 11, 2020

Uh oh!

Zastai commented Jun 11, 2020

Uh oh!

Zastai commented Jun 11, 2020

Uh oh!

jglick commented Jun 11, 2020

Uh oh!

jglick commented Jun 11, 2020

Uh oh!

Zastai commented Jun 11, 2020

Uh oh!

Zastai commented Jun 12, 2020

Uh oh!

jglick commented Jun 12, 2020

Uh oh!

Zastai commented Jun 13, 2020

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zastai commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jglick Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

Zastai Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Zastai commented Jun 6, 2020 •

edited

Loading

Zastai commented Jun 15, 2020 •

edited

Loading

Zastai Jun 24, 2020 •

edited

Loading

jglick Jun 24, 2020 •

edited

Loading