Support expanding variables in openqa-clone-job #6655
Conversation
okurz
force-pushed
the
feature/job_settings
branch
from
df0b34f to
af8a1d5
Compare
Related progress issue: https://progress.opensuse.org/issues/182645
Related progress issue: https://progress.opensuse.org/issues/182645
okurz
force-pushed
the
feature/job_settings
branch
from
af8a1d5 to
88d4ae2
Compare
| @@ -28,6 +30,9 @@ sub verify_incident_repos ($url_handler, $incident_repos) { | |||
| my @incident_urls; | |||
| my $ua = $url_handler->{ua}; | |||
| foreach my $incident (split(/,/, $incident_repos)) { | |||
| die | |||
There was a problem hiding this comment.
I don't think it is good idea , maybe just print warning instead ? It reminds me story around #6468 which showed us that it is not always good thing to decide on behalf of caller if it is make sense to continue . I still see some cases where no matter that incident repos are not expanded run still make sense . For example some VRs where I simply skip use of this variables
There was a problem hiding this comment.
I wouldn't mind the die. If we continue here we would probably just run into a hard failure later so failing fast is probably the best thing to do here.
There was a problem hiding this comment.
@Martchus what about the case when I clone job for some VR and simply skip install of updates ? We doing it quite often and just not bother about content of this variables
There was a problem hiding this comment.
And then you still use --check-repos and the checks pass despite the incomplete content of the variables? If that's the case then it should probably be a warning. Maybe you can give a concrete example of how you invoke openqa-clone-job so we're sure we're talking about the same thing.
There was a problem hiding this comment.
And then you still use --check-repos and the checks pass despite the incomplete content of the variables?
ah sorry I didn't realize that . so it will die only if --check-repos flag will be used ?
There was a problem hiding this comment.
I've just double-checked and code in CloneJobSUSE.pm is only executed if --check-repos is present.
There was a problem hiding this comment.
ok than I am agree that we need to die here
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6655 +/- ##
=======================================
Coverage 99.19% 99.19%
=======================================
Files 398 398
Lines 40800 40809 +9
=======================================
+ Hits 40472 40481 +9
Misses 328 328 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -28,6 +30,9 @@ sub verify_incident_repos ($url_handler, $incident_repos) { | |||
| my @incident_urls; | |||
| my $ua = $url_handler->{ua}; | |||
| foreach my $incident (split(/,/, $incident_repos)) { | |||
| die | |||
| "URL '$incident' contains an unexpanded variable. Specify the necessary variables at the command line for expansion. See https://open.qa/docs/#_variable_expansion for details" | |||
| if $incident =~ /%/; | |||
There was a problem hiding this comment.
The % sign is commonly used in URL encoding so this check seems quite wrong.
|
|
||
| our @EXPORT = qw(detect_maintenance_update); | ||
|
|
||
| sub collect_incident_repos ($url_handler, $settings) { | ||
| OpenQA::JobSettings::expand_placeholders($settings); |
There was a problem hiding this comment.
This function should probably return whether some placeholders could not be expanded. Don't do this by checking for a % as mentioned in the other comment.
There was a problem hiding this comment.
Ok. I suggest we do
first to ensure that the worker has no problems with the return value of expanding and then I can try to incorporate that here.
| @@ -28,6 +30,9 @@ sub verify_incident_repos ($url_handler, $incident_repos) { | |||
| my @incident_urls; | |||
| my $ua = $url_handler->{ua}; | |||
| foreach my $incident (split(/,/, $incident_repos)) { | |||
| die | |||
There was a problem hiding this comment.
I wouldn't mind the die. If we continue here we would probably just run into a hard failure later so failing fast is probably the best thing to do here.
| @@ -28,6 +30,9 @@ sub verify_incident_repos ($url_handler, $incident_repos) { | |||
| my @incident_urls; | |||
| my $ua = $url_handler->{ua}; | |||
| foreach my $incident (split(/,/, $incident_repos)) { | |||
| die | |||
| "URL '$incident' contains an unexpanded variable. Specify the necessary variables at the command line for expansion. See https://open.qa/docs/#_variable_expansion for details" | |||
There was a problem hiding this comment.
This needs a trailing \n, otherwise Perl prints a line number and it looks like a mistake in the program itself:
| "URL '$incident' contains an unexpanded variable. Specify the necessary variables at the command line for expansion. See https://open.qa/docs/#_variable_expansion for details" | |
| "URL '$incident' contains an unexpanded variable. Specify the necessary variables at the command line for expansion. See https://open.qa/docs/#_variable_expansion for details\n" |
Related progress issue: https://progress.opensuse.org/issues/182645
After: