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-61956] Adapt test to stricter enforcement of createFreeStyleProject arg #200

Merged
merged 4 commits into from
Jul 8, 2020

Conversation

jglick
Copy link
Member

@jglick jglick commented Jul 8, 2020

Compatibility with @calvinpark’s jenkinsci/jenkins#4684. Noticed in #199.

@jglick
Copy link
Member Author

jglick commented Jul 8, 2020

BTW I checked workflow-multibranch and saw some failures, but not obviously related. In particular I was worried about https://github.com/jenkinsci/workflow-multibranch-plugin/blob/388dedec6917d2c4ea4d7c583f3ddb465e92db06/src/test/java/org/jenkinsci/plugins/workflow/multibranch/WorkflowBranchProjectFactoryTest.java#L56-L70 but this passed against 2.237. That is because ComputedFolders go through a different mechanism to add children, so they are allowed to have children with for example % in the name even though checkGoodName would not tolerate that.

@calvinpark
Copy link

In my testing of the workflow-multibranch for jenkinsci/jenkins#4684, the "illegal characters" in checkGoogName were properly encoded therefore were not affected by the change jenkinsci/jenkins#4684 (comment)

For your remark on ComputedFolder, is that a false positive that should also run checkGoodName, or is it passing as it should?

@jglick
Copy link
Member Author

jglick commented Jul 8, 2020

the "illegal characters" in checkGoogName were properly encoded therefore were not affected by the change

I think you are misreading the situation. https://github.com/jenkinsci/jenkins/blob/0eca516e80de6ebd6183b3cca670d8f56c6f5500/core/src/main/java/jenkins/model/Jenkins.java#L4030 indeed rejects %, which for example would be in the Item.name arising from a branch containing a /, but it does not matter because ComputedFolder is using another mechanism to add generated children which bypasses your patch.

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dwnusbaum dwnusbaum merged commit b73c5a5 into jenkinsci:master Jul 8, 2020
@jglick jglick deleted the checkGoodName-JENKINS-61956 branch July 8, 2020 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants