-
Notifications
You must be signed in to change notification settings - Fork 372
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
Fix JobDSL requiring empty and UI-only values #254
Conversation
Change to fix one thing your break several others.
@darxriggs |
I can keep this PR in mind but cannot make any promises when I'll get around to have a look at it in more detail. |
Reporting user said this works as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
Outdated
Show resolved
Hide resolved
public GitHubSCMSource bindResolve(StaplerRequest staplerRequest, JSONObject json) { | ||
boolean configuredByUrl = json.getBoolean("configuredByUrl"); | ||
|
||
if(configuredByUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an extra space after if
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I so want to turn on automatic formatting on this project.
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
Show resolved
Hide resolved
@@ -37,6 +37,6 @@ protected void assertConfiguredAsExpected(RestartableJenkinsRule restartableJenk | |||
|
|||
@Override | |||
protected String stringInLogExpected() { | |||
return "Setting class org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.repoOwner = jenkins-infra"; | |||
return "Setting org.jenkinsci.plugins.github_branch_source.GitHubSCMSource{id='null'}.repoOwner = jenkins-infra"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that change required / where is it originating from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the toString()
is generated from some underlying library. Do you need me to go digging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString()
is from jenkins.scm.api.SCMSource
. I am just wondering why this was different before. I guess there are changes (e.g. configuration-as-code upgrade) merged from master
into this branch that are not easily visible in the GitHub UI.
* | ||
* @param repoOwner the repository owner. | ||
* @param repository the repository name. | ||
* @param repositoryUrl HTML URL for the repository. If specified, takes precedence over repoOwner and repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess HTML
should be HTTP
here.
@@ -413,10 +431,7 @@ public void setApiUri(@CheckForNull String apiUri) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An exception could be thrown to make this invalid usage more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we actually don't want to throw an exception here.
One the things that is most striking about this PR is how much of it goes against what we all agree is "good design" because it is serving the needs of data binding. This is another one of those cases. If this gets called more than once due to databinding we don't want to throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what scenarios are the setters called more than once? That seems surprising to me, is it related to bindResolve
? Otherwise, I would think that we definitely would want to throw exceptions as @darxriggs describes, otherwise, something like this ordering of events (only possible via incorrect data binding): setRepoOwner
, followed by setRepositoryUrl
, followed by setRepository
, would silently leave the GitHubSCMSource
in an invalid state. Is there a guaranteed order in which setters will be called?
|
||
try { | ||
info = GitHubRepositoryInfo.forRepositoryUrl(repositoryUrl); | ||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching a Throwable
is not a good idea. What's the intention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No matter what error you get from this method call, report it to the UI as a form validation error. It will always look better that way than the default UI exception formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, and e.getCause();
returns a throwable.
@@ -1996,37 +2078,54 @@ public FormValidation doCheckCredentialsId(@CheckForNull @AncestorInPath Item co | |||
public FormValidation doValidateRepositoryUrlAndCredentials(@CheckForNull @AncestorInPath Item context, | |||
@QueryParameter String repositoryUrl, | |||
@QueryParameter String credentialsId) { | |||
|
|||
// Start by verifying we have a well-formed GitHub API URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason to move this check before the permission check? Even if verifying a URL should not denote a security risk.
GHRepository repo = github.getRepository(info.getRepoOwner() + "/" + info.getRepository()); | ||
if (repo != null) { | ||
sb.append(" Connected to "); | ||
sb.append(repo.getHtmlUrl()); | ||
sb.append("."); | ||
} | ||
} catch (IOException e) { | ||
return FormValidation.error(e, "Error validating repository information. " + sb.toString()); | ||
return FormValidation.error(e, sb.toString() + " Error connecting to repository: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "successful" information in sb
in an error case that useful in the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sb
is built up along the way. It lets people understand how far verification got before failing.
@@ -371,14 +405,14 @@ public GitHubSCMSource(@CheckForNull String id, @CheckForNull String apiUri, | |||
@NonNull String checkoutCredentialsId, | |||
@CheckForNull String scanCredentialsId, @NonNull String repoOwner, | |||
@NonNull String repository) { | |||
this(repoOwner, repository, null, false); | |||
this(repoOwner, repository); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question here is if a with @Deprecated
annotated constructor should be used (even if this is a very valid one) or just the default one + setters. Otherwise code checkers / IDEs might complain about this fact.
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
Outdated
Show resolved
Hide resolved
I manually tested using this branch and
example-1JobDSLmultibranchPipelineJob('example-1') {
branchSources {
github {
id('1')
repoOwner('jenkinsci')
repository('job-dsl-plugin')
}
}
} Job XML Output...
<jenkins.branch.BranchSource>
<source class="org.jenkinsci.plugins.github_branch_source.GitHubSCMSource">
<id>1</id>
<scanCredentialsId/>
<checkoutCredentialsId>SAME</checkoutCredentialsId>
<repoOwner>jenkinsci</repoOwner>
<repository>job-dsl-plugin</repository>
<includes>*</includes>
<excludes/>
<buildOriginBranch>true</buildOriginBranch>
<buildOriginBranchWithPR>true</buildOriginBranchWithPR>
<buildOriginPRMerge>false</buildOriginPRMerge>
<buildOriginPRHead>false</buildOriginPRHead>
<buildForkPRMerge>true</buildForkPRMerge>
<buildForkPRHead>false</buildForkPRHead>
</source>
<strategy class="jenkins.branch.DefaultBranchPropertyStrategy">
<properties class="empty-list"/>
</strategy>
</jenkins.branch.BranchSource>
... example-2JobDSLmultibranchPipelineJob('example-2') {
branchSources {
branchSource {
source {
github {
id('2')
repoOwner('jenkinsci')
repository('job-dsl-plugin')
}
}
}
}
} Job XML Output...
<jenkins.branch.BranchSource plugin="branch-api@2.1.2">
<source plugin="github-branch-source@2.5.9-SNAPSHOT" class="org.jenkinsci.plugins.github_branch_source.GitHubSCMSource">
<id>2</id>
<apiUri>https://api.github.com</apiUri>
<repoOwner>jenkinsci</repoOwner>
<repository>job-dsl-plugin</repository>
<traits/>
</source>
</jenkins.branch.BranchSource>
... example-3JobDSLmultibranchPipelineJob('example-3') {
branchSources {
branchSource {
source {
github {
id('3')
repositoryUrl('https://github.com/jenkinsci/job-dsl-plugin.git')
}
}
}
}
} Job XML Output...
<jenkins.branch.BranchSource plugin="branch-api@2.1.2">
<source plugin="github-branch-source@2.5.9-SNAPSHOT" class="org.jenkinsci.plugins.github_branch_source.GitHubSCMSource">
<id>3</id>
<apiUri>https://api.github.com</apiUri>
<repoOwner/>
<repository/>
<traits/>
</source>
</jenkins.branch.BranchSource>
... |
I digged a bit deeper regarding the failing With some debugging and changing the log level for The job is not created correctly via Job DSL because of the customizations in call chain log
This functionality is completely new to me. Maybe @dwnusbaum and @joseblas can assist here as they have been involved in this customization. |
* @param repositoryUrl HTTP URL for the repository. | ||
* @since 2.2.0 | ||
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really required to be deprecated or just not intended to be used for data-binding? It could be used programatically without problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be used, but we do not want that. Also, as far as I know, there aren't any other valid uses for these constructors aside from data binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be ok to leave it undeprecated, even if just for use in tests to avoid misuse of the data binding constructor. Same with new GitHubSCMSource(String, String)
. If you leave it deprecated, you will get community PRs that convert all of the tests to use the non-deprecated constructor, which may or may not be what you want.
@@ -353,7 +362,32 @@ public GitHubSCMSource(String repoOwner, String repository, String repositoryUrl | |||
*/ | |||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really required to be deprecated or just not intended to be used for data-binding? It could be used programatically without problems.
@darxriggs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok in general, I added some minor comments. I am a bit surprised by your comment that the setters can be called twice in some scenarios, so I am going to do a bit of testing.
* @param repositoryUrl HTTP URL for the repository. | ||
* @since 2.2.0 | ||
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be ok to leave it undeprecated, even if just for use in tests to avoid misuse of the data binding constructor. Same with new GitHubSCMSource(String, String)
. If you leave it deprecated, you will get community PRs that convert all of the tests to use the non-deprecated constructor, which may or may not be what you want.
repoOwner = ""; | ||
repository = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty weird, might mask some bugs. Personally, I would rather see an NPE caused by the instance being in an illegal state than have the instance silently do the wrong thing without any way for us to diagnose the issue.
Maybe assign the fields to null and add SuppressFBWarnings
mentioning that the setters will make the fields non-null in practice, or change the annotation to @CheckForNull
(but that's going to require a bunch of changes in the methods here). Same thing for the defaultIfBlank
calls in the setters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwnusbaum Community PR to do that conversion are fine with me. 😄
As to the assigning blank, you and @darxriggs both commented on it. And you're right, but ... this exactly the point of this PR - DataBinding with a complex set of parameters basically demands that we construct the instance in non-ready-to-run state. On the other hand, for any valid use of of an instance these values will not be null. On the other other hand, we don't make them null by default because we want to be able to load/save instances that are in a non-ready-to-run state so that we can modify/fix them to be ready-to-run.
@@ -413,10 +431,7 @@ public void setApiUri(@CheckForNull String apiUri) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what scenarios are the setters called more than once? That seems surprising to me, is it related to bindResolve
? Otherwise, I would think that we definitely would want to throw exceptions as @darxriggs describes, otherwise, something like this ordering of events (only possible via incorrect data binding): setRepoOwner
, followed by setRepositoryUrl
, followed by setRepository
, would silently leave the GitHubSCMSource
in an invalid state. Is there a guaranteed order in which setters will be called?
this.setRepositoryUrl(json.get("repositoryUrl").toString()); | ||
} else { | ||
this.repositoryUrl = null; | ||
this.apiUri = json.get("apiUri").toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.apiUri = json.get("apiUri").toString(); | |
this.apiUri = setApiUri(json.get("apiUri").toString()); |
Otherwise we never go through normalizeApiUri
on this path.
if (e instanceof IllegalArgumentException && e.getCause() != null) { | ||
e = e.getCause(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? If it's to try to make it easier for users to understand the stack trace, I would delete these lines, and change the next line from e.getMessage
to something like The format of the repository URL is invalid. Please check the help text and adjust the URL
, which is probably more helpful unless we are dealing with a bug.
Also though, if the changes in doValidateRepositoryUrlAndCredentials
aren't required for the main fix, it might be better to move it to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, you've both said this less than ideal. I see a way to clean it up.
This allows repositoryUrl to be passed in via JobDSL and templating, but filters it out on round trip.
4b6100d
to
5474da9
Compare
Short answer, no. If there is an order it is incidental. |
I was hoping to include this in today's release, but discussion indicates there's more work to be done here. |
JENKINS issue(s):
Description:
bindResolve
to access the passed JSON and select which settings to use. For JobDSL, it lets the caller provide the information they choose.Documentation changes:
Users/aliases to notify: