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

Filter embargoed also by Flavor #164

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Conversation

mimi1vx
Copy link
Member

@mimi1vx mimi1vx commented Feb 8, 2024

@mimi1vx mimi1vx requested a review from baierjan February 8, 2024 16:21
@mimi1vx
Copy link
Member Author

mimi1vx commented Feb 8, 2024

@asmorodskyi

Copy link
Contributor

@asmorodskyi asmorodskyi left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -8,7 +8,7 @@ def test_aggregate_constructor():
needed by the constructor?
"""
config = {}
config["FLAVOR"] = None
config["FLAVOR"] = "None"
Copy link
Member

Choose a reason for hiding this comment

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

I guess FLAVOR is actually a key name from metadata so in reality it cannot be None so the change to string make sense.

@mergify mergify bot merged commit fbd4bc1 into openSUSE:master Feb 8, 2024
3 checks passed
@mimi1vx mimi1vx deleted the poo153334 branch February 8, 2024 17:19
return any(k.startswith("PUBLIC") for k in self.settings.keys())
def filter_embargoed(self, flavor: str) -> bool:
return any(k.startswith("PUBLIC") for k in self.settings) or any(
flavor.startswith(s) for s in ("Azure", "EC2", "GCE")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is using of startswith too restrictive?

It will works as expected but I'm not sure there's any "official rule" that say that name of the cloud provider has to be used at the beginning of the FLAVOR name.

More deeply in the PC test lib there's a more coded convention based on PUBLIC_CLOUD_PROVIDER and not on FLAVOR

There's a PC lib setting named PUBLIC_CLOUD_PROVIDER that has a coded naming convention: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/2e0b43a6f0cea5d2e8856e2a35d8bb23bb625325/lib/publiccloud/basetest.pm#L33

Then, at least for the QE-SAP jobs the connection between FLAVOR and PUBLIC_CLOUD_PROVIDER happens at JobGroup level (have a look for the setting name in JobGroup file openQA/yaml_job_groups/maintenance/maintenance_sle15_sp5_sap.yml in repo openqa_ha_sap

Copy link
Contributor

Choose a reason for hiding this comment

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

the problem which @mimi1vx was trying to address here that currently https://gitlab.suse.de/qa-maintenance/metadata does not have PUBLIC_CLOUD_PROVIDER for SAP related products . And if you will add it there than you won't need second part at all . Because any(k.startswith("PUBLIC") for k in self.settings) would already return true for PUBLIC_CLOUD_PROVIDER variable no matter what value it has

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but those files not only have Cloud tests in them, adding a dummy setting on the top like

product: SAP-15-SP5
settings:
  DISTRI: sle
  VERSION: '15-SP5'
  PUBLIC_DONALDUCK: 'Rockerduck'

will result influencing also the non cloud tests managed by that file.

Could you confirm it?

def filter_embargoed(self) -> bool:
return any(k.startswith("PUBLIC") for k in self.settings.keys())
def filter_embargoed(self, flavor: str) -> bool:
return any(k.startswith("PUBLIC") for k in self.settings) or any(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here it is safe to have startswith but PUBLIC can be extended to PUBLIC_CLOUD

@mpagot
Copy link
Contributor

mpagot commented Feb 8, 2024

I'd like to propose a possible different approach to drastically simplify logic in filter_embargoed. What about to add a field in the metadata to opt-in the check?

    Azure-SAP-PAYG-Incidents:
      embargo_sensible: true       <<<-----
      aggregate_job: false
      archs:
        - x86_64
      issues: *test_issues_list
      required_issues: *required_list
      packages: *whitelist_ha_hana
      params_expand:
        PUBLIC_CLOUD_IMAGE_ID: 'suse:sles-sap-15-sp5:gen2:latest'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants