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-34410] Improved the search procedure of through SCMSourceCriteria #50
Conversation
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. |
…h SCMScourceCriteria
No strong opinion, but by design #50 is better as |
This is a strong opinion since you have two alternatives and choose one of them. In the ticket description there are two positive things. IMO, this alternative is still better. |
Exactly, I reply to this question and additionally, provide more contextual information to cover specific scenarios. |
Well, not really, as I'm ok with this to be merged too (it's an opinion, but not strong 😄 ) |
🐝 if you value my opinion |
@stephenc your opinion is always welcome! |
Although this PR is ready to be merged, I prefer to wait the @jglick's opinion. |
Huh? This is #50! Did you mean an alternative to jenkinsci/workflow-multibranch-plugin#3? |
return true; | ||
} | ||
if (content.getName().equalsIgnoreCase(path)) { | ||
listener.getLogger().format(" %s not found (but found %s, search is case sensitive) in this %s, skipping", path, content.getName(), thing); |
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.
Might want to use ‘%s’
to make it clearer that we are quoting pathnames here.
🐝 assuming you tried it and it seems to behave nicely. Looks right. |
My mistake, too many PRs at the same time 😵 |
Please, find an example:
|
@jglick Your re-bee would be fine if you agree with my last commit. |
} | ||
listener.getLogger().format(" %s does not exist in this %s%n", path, thing); |
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.
Might want to put quotes around the pathname here for consistency with the other two messages.
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.
@jglick I did some tests:
|
} | ||
if (content.getName().equalsIgnoreCase(filename)) { | ||
listener.getLogger().format(" ‘%s’ not found (but found ‘%s’, search is case sensitive) in this %s, skipping%n", path, content.getName(), thing); | ||
return false; |
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.
🐜 not quite right, since if you have a repo with both Jenkinsfile
and jenkinsfile
you would want to return true, even if we encountered jenkinsfile
first. Unlikely, but at any rate would be more correct to delete this statement and let it fall through to the end of the method.
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.
@jglick I thought about it but I decided not to considerer it. As you always say Do not add code if there are not real use cases at least you say that for new API, etc...
This PR improves a real use case that I found few weeks ago.
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 agree, this is a corner case easily diagnosable by the user looking at the logs.
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 brought it up mainly because it is a regression: before this PR the branch would correctly be recognized, now it will not. As noted, it is unlikely to occur, and easily enough diagnosed.
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.
@jglick I prefer the positive things that this PR provided than the hypothetical issue it would be having a repository with both Jenkinsfile
and jenkinsfile
.
🐝 |
re-:bee: |
* Initialize transient fields after XStream deserialization * Re-wrote test to actually restart Jenkins * Replace RestartableJenkinsRule.addStep(Statement) with .then(Step)
JENKINS-34410
This is an alternative to jenkinsci/workflow-multibranch-plugin#3
@reviewbybees