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-32526] - Handle null @AncestorInPath when selecting Jobs/Dow… #79

Merged
merged 5 commits into from Apr 1, 2016

Conversation

Projects
None yet
5 participants
@Dohbedoh
Copy link
Contributor

commented Mar 7, 2016

…nstream/Upstream Jobs and for autocompletion.

JENKINS-32526

[JENKINS-32526] - Handle null @AncestorInPath when selecting Jobs/Dow…
…nstream/Upstream Jobs and for autocompletion.
@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 7, 2016

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.

@@ -577,7 +577,9 @@ public String resolve(String name) {
public static final class DescriptorImpl extends BuildStepDescriptor<Builder> {

public FormValidation doCheckProjectName(
@AncestorInPath Job<?,?> anc, @QueryParameter String value) {
@AncestorInPath AbstractProject anc, @QueryParameter String value) {

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 7, 2016

Member

Why do you need have it AbstractProject?
Pipeline (aka. workflow)'s snippet generator requires Job.
f28c554

import hudson.model.StringParameterDefinition;
import hudson.model.StringParameterValue;
import hudson.model.User;
import hudson.model.Result;

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 7, 2016

Member

I don't like wildcard import. (especially for classes)
Do you actually need this change?

This comment has been minimized.

Copy link
@Dohbedoh

Dohbedoh Mar 12, 2016

Author Contributor

No sorry. That's the IDE that organized import automatically.


assertArrayEquals(expectedValues, d.doAutoCompleteUpstreamProjectName(value, project).getValues().toArray());
//JENKINS-32526
assertArrayEquals(expectedValues, d.doAutoCompleteUpstreamProjectName(value, null).getValues().toArray());

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 7, 2016

Member

It's better to sort the list or wrap it with Set.
AutoCompleteCandidates doesn't look ensure the order of values (strange behavior!)

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 7, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

[JENKINS-32526] - Fixed imports and ordered set, changed attribute cl…
…ass back to Job, added message about the context
@@ -188,6 +188,11 @@ public FormValidation doCheckUpstreamProjectName(
if (containsVariable(upstreamProjectName)) {
return FormValidation.ok();
}

if (project == null) {

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

While you are it, you should replace AbstractProject with Job to get Pipeline compatibility missed in #55. Ditto in other related validation methods. CC @apemberton: currently with Pipeline this throws a NullPointerException; fixing the PR as written would make it unconditionally return OK; also fixing the type of the parameter would make it actually perform the desired validation.

This comment has been minimized.

Copy link
@jglick

jglick Mar 16, 2016

Member

JENKINS-33578 specifically.

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

@jglick Changed the variable type to Job. I needed to workaround a call to AbstractProject#getRootProject() in the doCheckUpstreamProjectName and doCheckUpstreamBuildNumber functions.

if(project != null && project instanceof AbstractProject) {
upstreamRoot = ((AbstractProject) project).getRootProject();
} else {
upstreamRoot = null;

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

No, you want to set it to project. (getRootProject defaults to returning this.)


AbstractProject<?,?> upstreamProject = jenkins.getItem(
upstreamProjectName, project.getRootProject(), AbstractProject.class
upstreamProjectName, upstreamRoot, AbstractProject.class

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

And here you want to deal with Job, not AbstractProject.

if(project != null && project instanceof AbstractProject) {
rootProject = ((AbstractProject) project).getRootProject();
} else {
rootProject = null;

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

ditto


AbstractProject<?,?> upstreamProject = jenkins.getItem(
upstreamProjectName, project.getRootProject(), AbstractProject.class
upstreamProjectName, rootProject, AbstractProject.class

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

ditto

@@ -292,7 +306,9 @@ public AutoCompletionCandidates doAutoCompleteUpstreamProjectName(
@AncestorInPath AbstractProject<?,?> project

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

Need to make this Job.

@@ -292,7 +306,9 @@ public AutoCompletionCandidates doAutoCompleteUpstreamProjectName(
@AncestorInPath AbstractProject<?,?> project
) {
// Specified Item to allow to autocomplete folders (maybe confusing...).
return AutoCompletionCandidates.ofJobNames(Item.class, value, project, project.getParent());
return project == null
? AutoCompletionCandidates.ofJobNames(Item.class, value, null)

This comment has been minimized.

Copy link
@jglick

jglick Mar 17, 2016

Member

If there is no context, just turn off completions (return new AutoCompletionCandidates()).

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

BTW if you want to test Pipeline compatibility of completions, WorkflowJob is already in the test classpath. As I noted to @apemberton in JENKINS-33577, it is not really useful in this case anyway: DownstreamBuildSelector will not actually work.

@@ -1,3 +1,4 @@
CopyArtifact.AncestorIsNull=Ancestor/Context Unknown: the project specified cannot be validated

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 17, 2016

Member

I don't think this is helpful for users.
How about a text like "You look configuring non-buildable job (may be template) ..."?

This comment has been minimized.

Copy link
@jglick

jglick Mar 18, 2016

Member

Well it is not exactly “non-buildable”. But certainly the term “ancestor” will be meaningless to a user (this is an API term). I think it suffices to say that the configuration form is being displayed without enough context to determine which job will host the build step.

@ikedam

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

You'd better add a comment when you add a new commit as github doesn't send a notification for commits.

@@ -196,7 +196,7 @@ public FormValidation doCheckUpstreamProjectName(
}

AbstractProject<?,?> upstreamProject = jenkins.getItem(
upstreamProjectName, project.getRootProject(), AbstractProject.class
upstreamProjectName, project, AbstractProject.class

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 24, 2016

Member

You might misunderstand the comment by jglick (https://github.com/jenkinsci/copyartifact-plugin/pull/79/files/65b30d6da1983e11be0f93d9defb664796fca99d#r56505288)
That meant you don't need to use null for project even when project doesn't have getRootProject.
getRootProject should be still required if it's available.

Though I don't know an actual case resulting project.getRootProject() != project in this context, it's better to implement in the same way as DownstreamBuildSelector works.

@@ -177,7 +177,7 @@ protected boolean containsVariable(String str) {
* @return
*/
public FormValidation doCheckUpstreamProjectName(
@AncestorInPath AbstractProject<?,?> project,
@AncestorInPath Job<?,?> project,

This comment has been minimized.

Copy link
@ikedam

ikedam Mar 24, 2016

Member

You should skip the validation if project is null (that is, when configuring a template).

@Dohbedoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2016

@ikedam You're right I misunderstood jglick's point. I reimplemented the use of getRootProject and skip the validation in case project is null.
@reviewbybees

@ikedam ikedam referenced this pull request Mar 27, 2016

Merged

[JENKINS-33389] Adapt to new parent POM and split JTH #78

6 of 6 tasks complete
@ikedam

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

Thanks for the update.
Looks good to me.

@ikedam ikedam merged commit a34592b into jenkinsci:master Apr 1, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.