Project

General

Profile

action #63883

openqa-clone-job does not support removing an unuseful setting

Added by Xiaojing_liu 3 months ago. Updated 4 days ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Feature requests
Target version:
-
Start date:
2020-02-27
Due date:
% Done:

0%

Estimated time:
Difficulty:
Duration:

Description

User story

Sometimes users need to remove some settings when trying to clone a new job. For now, if users do not want some setting make sense, they will set a invalid value to those setting.

Acceptance criteria

  • when executing openqa-clone-job, users could remove some settting by set KEY= or --delete_settings KEY1,KEY2,...

Related issues

Related to openQA Project - action #63565: The extra setting is added to the new job when cloning a jobResolved2020-02-19

Related to openQA Project - action #65750: [17/04/2020 07:56:17] <DimStar> okurz: Martchus_ would appreciate if you could gave a look at tw snapshot 0415... dozens of incomplete tests (attempting to retrigger comolains about missing assets)Resolved2020-04-17

History

#1 Updated by okurz 3 months ago

  • Target version set to future

I know this is based on a chat with mkittler but I am not sure what the real problem was because setting a value to an empty value works with e.g. FOO=. Maybe the problem was only when using --parental-inheritance?

#2 Updated by Xiaojing_liu 3 months ago

okurz wrote:

I know this is based on a chat with mkittler but I am not sure what the real problem was because setting a value to an empty value works with e.g. FOO=. Maybe the problem was only when using --parental-inheritance?

I did a test again and look up the code. When we set FOO=, the setting FOO will be really deleted by this function clone_job_apply_settings. But when creating a new job, the create function will generate the setting again. Maybe the create function in API/V1/Job.pm should simply create a new job using the parameters when users clone jobs. This is also the question that mentioned in #63565.

#3 Updated by Xiaojing_liu 3 months ago

  • Related to action #63565: The extra setting is added to the new job when cloning a job added

#4 Updated by cdywan 2 months ago

  • Status changed from New to In Progress
  • Assignee set to Xiaojing_liu
  • Target version changed from future to Current Sprint

Xiaojing_liu wrote:

I did a test again and look up the code. When we set FOO=, the setting FOO will be really deleted by this function clone_job_apply_settings. But when creating a new job, the create function will generate the setting again. Maybe the create function in API/V1/Job.pm should simply create a new job using the parameters when users clone jobs. This is also the question that mentioned in #63565.

While this isn't a concrete bug I second Jane this should have been consistent to begin with. The special-case of an empty value is missing here because it's two code paths.

#6 Updated by cdywan 2 months ago

  • Status changed from In Progress to Feedback

PR was merged.

#8 Updated by okurz about 1 month ago

  • Related to action #65750: [17/04/2020 07:56:17] <DimStar> okurz: Martchus_ would appreciate if you could gave a look at tw snapshot 0415... dozens of incomplete tests (attempting to retrigger comolains about missing assets) added

#9 Updated by cdywan about 1 month ago

  • Status changed from Feedback to In Progress

Semi-related, os-autoinst/openQA#2931 which was inspired by this, in fact uncovered an apparent bug (in existing unit tests) as well as a yet to be investigated bug (in Tumbleweed Snapshot tests, see #65750).

#11 Updated by Xiaojing_liu about 1 month ago

  • Status changed from In Progress to Feedback

PR has been merged

#12 Updated by acarvajal 26 days ago

Hello,

I believe https://github.com/os-autoinst/openQA/pull/2959 introduced a regression that makes openQA perform the replacement of placeholders in settings before ensuring the value of DISTRI is lower case. This caused some tests to fail to find their qcow2 images referenced by HDD_1 which in turn has a %DISTRI% as part of its value.

For example: https://openqa.suse.de/tests/4189650
Last good: https://openqa.suse.de/tests/4174255

Not aware of other problems, but any setting with %DISTRI% must have been affected.

Call to OpenQA::JobSettings::expand_placeholders(\%settings); was originally after $settings{DISTRI} = lc($settings{DISTRI}); in L548 of OpenQA::Schema::Result::ScheduleProducts, but now this call is in OpenQA::JobSettings::generate_settins(\%params); which is called before.

#13 Updated by cdywan 18 days ago

acarvajal wrote:

For example: https://openqa.suse.de/tests/4189650
Last good: https://openqa.suse.de/tests/4174255

Not aware of other problems, but any setting with %DISTRI% must have been affected.

Call to OpenQA::JobSettings::expand_placeholders(\%settings); was originally after $settings{DISTRI} = lc($settings{DISTRI}); in L548 of OpenQA::Schema::Result::ScheduleProducts, but now this call is in OpenQA::JobSettings::generate_settins(\%params); which is called before.

I think os-autoinst/openQA#3045 should be addressing that? It was merged on Thursday

#14 Updated by Xiaojing_liu 18 days ago

cdywan wrote:

acarvajal wrote:

For example: https://openqa.suse.de/tests/4189650
Last good: https://openqa.suse.de/tests/4174255

Not aware of other problems, but any setting with %DISTRI% must have been affected.

Call to OpenQA::JobSettings::expand_placeholders(\%settings); was originally after $settings{DISTRI} = lc($settings{DISTRI}); in L548 of OpenQA::Schema::Result::ScheduleProducts, but now this call is in OpenQA::JobSettings::generate_settins(\%params); which is called before.

I think os-autoinst/openQA#3045 should be addressing that? It was merged on Thursday

yes, #3045 is used to fix that.

#15 Updated by acarvajal 18 days ago

cdywan wrote:

I think os-autoinst/openQA#3045 should be addressing that? It was merged on Thursday

Checking at some jobs for settings that use %DISTRI%, problem does seem fixed. Thanks a lot.

#16 Updated by Xiaojing_liu 4 days ago

  • Status changed from Feedback to Resolved
  • Target version deleted (Current Sprint)

Also available in: Atom PDF