Extend variable expansion to consider variables from worker config #6047
Conversation
* Allow substituting placeholders that refer to variables that are only defined in `workers.ini` for the particular worker slot that picked up the job * Keep placeholders of non-existing keys when doing the variable substitution during job creation on the web UI side (instead of removing them) so the worker still sees the placeholder and can do a final substitution (removing placeholders of non-existing keys, so the overall behavior does not change) * Do *not* propagate settings from the final substitution back to the web UI as the job might be restarted and then run on a different worker where settings might be different * Extend relevant documentation * See https://progress.opensuse.org/issues/169159
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6047 +/- ##
=======================================
Coverage 98.98% 98.98%
=======================================
Files 395 395
Lines 39425 39436 +11
=======================================
+ Hits 39025 39036 +11
Misses 400 400 ☔ View full report in Codecov by Sentry. |
| the string will be substituted with the value of the specified variable at that time. | ||
| The variable expansion applies to job settings defined in test suites, machines, | ||
| products and job templates. It also applies to job settings specified when | ||
| creating a single set of jobs and to variables specified in the worker config. |
There was a problem hiding this comment.
| creating a single set of jobs and to variables specified in the worker config. | |
| creating a single set of jobs and to variables specified in the worker config at the time of job exectution. |
There was a problem hiding this comment.
not sure if this applies, if the above is added.
There was a problem hiding this comment.
I wanted to keep it short. Our documentation is long enough and the addition seems redundant.
| Any job setting can refer to another variable using this syntax: `%NAME%`. When | ||
| the test job is created, the string will be substituted with the value of the | ||
| specified variable at that time. |
There was a problem hiding this comment.
| Any job setting can refer to another variable using this syntax: `%NAME%`. When | |
| the test job is created, the string will be substituted with the value of the | |
| specified variable at that time. | |
| openQA supports variable expansion, surrounding a test variable with percentage signs (`%`), e.g `%NAME%`. | |
| Variable expansion can happen only at two stages: | |
| 1. When the job is created the string will be substituted with the value of the | |
| specified variable at that time. This is useful to handle static information, like a directory or a repository. | |
| 1. Before the download state of the worker, making it a runtime value, that will not be shown in the openQA webUI, but is shown in the logs. i.e runtime provisioning of ephemeral values, or using different values defined at the worker level. |
There was a problem hiding this comment.
I would go as far as to say that also protected values like SECRET or _PASSWORD would also be a great case for https://progress.opensuse.org/issues/109019 and https://progress.opensuse.org/issues/105624
but for now that's a random crazy idea
There was a problem hiding this comment.
I wouldn't make this so long and avoid generic expressions like "the string". However, maybe I'll incorporate some parts of it.
| sub _expand_placeholder ($settings, $key, $start, $end, $visited_placeholders_in_parent_scope) { | ||
| return '' unless defined $settings->{$key}; | ||
| sub _expand_placeholder ($settings, $key, $start, $end, $visited_placeholders_in_parent_scope, $on_web_ui) { | ||
| # handle %CASEDIR% only on web UI; on the worker it is handled by _engine_workit_step_2 separately |
There was a problem hiding this comment.
This should be added in the documentation above
There was a problem hiding this comment.
No, it is already documented in a more fitting section.
|
@Martchus I won't block on docs, feel free to create follow ups if you see fit. |
defined in
workers.inifor the particular worker slot that picked upthe job
substitution during job creation on the web UI side (instead of removing
them) so the worker still sees the placeholder and can do a final
substitution (removing placeholders of non-existing keys, so the overall
behavior does not change)
UI as the job might be restarted and then run on a different worker where
settings might be different
@foursixnine @okurz I have not implemented a default value as suggested in https://progress.opensuse.org/issues/169159 to avoid further complexity. This has already turned out to be a little bit more involved than I expected without it. However, let me know if you need it. Since we're talking about OSD here where all workers are Salt-controlled it is not a big deal to simply set the variable on all workers, though. Alternatively one could also set a default within OSADO unless this is also about ISO/HDD assets (because for those the cache service already needed to know the default).