Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upMake SCMBinder try to abort builds in which Jenkinsfile has been modified by an untrusted contributor #69
Conversation
|
Change looks fine code-wise, but I want a little time to think about whether this is actually the right behavior. |
|
Agree with @abayer that this needs some extra consideration for unexpected impacts |
reviewbybees
commented
Jan 11, 2018
|
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. |
|
@rsandell that would be a reasonable enhancement, although the link could only safely be clicked by someone with Anyway that is something that could be added later—this PR at least fixes the current confusing behavior, and does so with minimal code changes. |
|
I'm still not 100% on this - I'd expect to see bug reports trickle in once this is released from people not expecting their PR build to fail because it also happened to contain a change to the |
My attitude is that this is better than silently ignoring the change. You can always request that a maintainer refile the same branch. Or if JENKINS-46795 were implemented, it would suffice for the maintainer to approve the PR and kick off a new build. |
|
Just remember that this was mentioned in JENKINS-45970. |
|
@jglick Yeah, I think I might prefer loud logging in these cases rather than aborting, as mentioned in JENKINS-45970. |
|
Well decide what you want one way or another and the PR can be adjusted accordingly. The worst thing you can do is leave things the way they are. |
|
@jglick Yeah, let's go with logging a loud warning, then. =) |
|
OK. I still think a simple build failure is a lot more discoverable than any kind of build logging, but will do whatever is necessary to get something merged. |
| public static class WarningNote extends ConsoleNote { | ||
|
|
||
| @Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) { | ||
| text.addMarkup(0, text.length(), "<span class='warning-inline'>", "</span>"); |
This comment has been minimized.
This comment has been minimized.
|
Would add @dwnusbaum as a reviewer if I had write permission. |
|
Huh, I don't have write access either. That's...wrong. |
|
And fixed - the developers group didn't have write access for some reason. |
jglick commentedDec 18, 2017
jenkinsci/ssh-slaves-plugin#79 and the like are pretty annoying: the historical behavior of
SCMBinderis to allow a build which includes an untrusted edit toJenkinsfileto proceed, but actually reading theJenkinsfilecontents from the corresponding trusted revision instead. If you get a green check mark, no one is going to go looking for the one-line warning about this. It is better to behave as if the script started withreadTrusted 'Jenkinsfile'In other words, to just fail immediately in such a case. I cannot think of any good reason for anyone to be relying on the old behavior, but just in case I kept it as an option which can be enabled by system property.
Note that this change will not have any effect on GitHub PR merge jobs until JENKINS-43194 is implemented. Performing a rev-to-tip comparison of script contents in a heavyweight checkout would be pretty awkward, as
SCMBindercould not then rely onCpsScmFlowDefinitionfor script checkout;ReadTrustedStepwould need to be refactored to make its main logic reusable, and there would be a lot of different code paths to maintain code coverage for. I think the effort is better put into JENKINS-43194.@reviewbybees