Validate the test name when creating new jobs #6195
Conversation
Martchus
commented
Feb 20, 2025
Martchus
commented
- Use the same regex we already use when validating the YAML schema for job templates
- See https://progress.opensuse.org/issues/177267
* Use the same regex we already use when validating the YAML schema for job templates * See https://progress.opensuse.org/issues/177267
Martchus
force-pushed
the
validate-test-name
branch
from
98d3a34 to
00264e5
Compare
lib/OpenQA/Jobs/Constants.pm
Outdated
| use constant TEST_NAME_REGEX => qr/^[A-Za-z 0-9_*.+-]+$/; | ||
| # note: In contrast to the YAML schema a few more characters are allowed here as they are useful for manually | ||
| # triggered jobs, e.g. via `openqa-clone-custom-git-refspec`. | ||
| use constant TEST_NAME_REGEX => qr|^[A-Za-z 0-9_*.+,:/#@%"'-]+$|; |
There was a problem hiding this comment.
I would not allow %'". Are they actually used?
I only saw " being used on osd, but only a handful cases, and I think it can be rather confusing, and it creates hard to read command lines if you have to quote something that can contain quotes itself.
There was a problem hiding this comment.
Also you can disallow a trailing newline by using \z instead of $:
| use constant TEST_NAME_REGEX => qr|^[A-Za-z 0-9_*.+,:/#@%"'-]+$|; | |
| use constant TEST_NAME_REGEX => qr|^[A-Za-z 0-9_*.+,:/#@%"'-]+\z|; |
There was a problem hiding this comment.
Yes, % is used, e.g. Chubykalo_welcome_to_wsl_poo%23176676. However, I guess people would probably be able to live without it.
I added ' because " was used and I think if we allow double-quotes we should also allow single-quotes for consistency. However, maybe you're right and we should not allow quotes at all. The only occurrence with quote that comes to mind is sles4sap_gnome_saptune_delete_rename"rbmarliere" and maybe people can also here live without it. Maybe this was actually even a mistake again.
@okurz What is your take on that?
There was a problem hiding this comment.
to me it looks like poo%23176676 could be a typo and was meant to be poo#23176676
There was a problem hiding this comment.
I apparently missed a lot of the conversation. You asked me for my take on that but I now see that the test name regex only allows A-Za-z so really only ASCII alphabet characters? I see the main problem within https://progress.opensuse.org/journals/910146/diff?detail_id=853467 which adds as AC1 the opposite of what the ticket subject says. If I am not mistaken Martchus and me suggested a blocklist, not a passlist. I suggest that we either go back to the decision step what we want to achieve or you better clarify in the git commit and PR what the reasoning is behind this
As mentioned on https://progress.opensuse.org/issues/177267, the use of `\s` was probably a mistake.
* Allow more special characters including the colon (the `openqa-clone-job` script will have to cope with it by only splitting the key/testname on the first colon) * Keep `=` and tabs disallowed; the production jobs using those characters are most likely created by accident * See https://progress.opensuse.org/issues/177267
Martchus
force-pushed
the
validate-test-name
branch
from
00264e5 to
6cbf8d9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6195 +/- ##
=======================================
Coverage 98.99% 98.99%
=======================================
Files 396 396
Lines 39785 39795 +10
=======================================
+ Hits 39386 39396 +10
Misses 399 399 ☔ View full report in Codecov by Sentry. |
