[JENKINS-51248] Added label and container regex to fail job if bad #332
Conversation
|
I don't like the fact that a lot of setters throw exceptions and get propagated all over the place I have asked around and got this advice
|
| public void setLabel(String label) throws Exception { | ||
| if(!PodTemplateUtils.validateLabel(label)) | ||
| { | ||
| throw new Exception(String.format("Labels must follow required specs - https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set - %s", label)); |
There was a problem hiding this comment.
Why not IllegalArgumentException?
There was a problem hiding this comment.
Correct - removed it and replaced with above advice.
| @@ -81,6 +81,10 @@ public ContainerTemplate(String name, String image, String command, String args) | |||
|
|
|||
| @DataBoundSetter | |||
| public void setName(String name) { | |||
| if(!PodTemplateUtils.validateContainerName(name)) | |||
| { | |||
| throw new IllegalArgumentException(String.format("Container Names MUST match RFC 1123 - They can only contain lowercase letters, numbers or dashes. ContainerName-%s",name)); | |||
There was a problem hiding this comment.
Fine but you should then add FormValidation DescriptorImpl.doCheckName(@QueryParameter String name) so that users of Pipeline Syntax will see that a proposed step configuration is invalid.
MattLud
force-pushed
the
JENKINS-51248
branch
2 times, most recently
from
44bbb2c to
3cc14a0
Compare
|
Updated to remove grossy unnecessary exception declarations. |
carlossg
changed the title
Move pipeline tests that don't need kubernetes to a new class
When parsing the yaml
Need to pass it explicitly because if ip does not exist tests will fail with
java.net.BindException: Cannot assign requested address
Initial approach to providing validation and immediate job failure on bad label and name.
Please review correctness of regex and if throwable Exception should be changed.