Refactor get_suse_container_urls using data structure for each distri #13141

b10n1k · 2021-08-27T08:43:14Z

I have approach this problem organizing the data into a hash. The data
structure can hold information explicitly, so it is more ease to handle.

Each hold three information

released_registry
totest_registry
available_arch

I use anonumous functions for registries to provide flexibility. For instance
you can choose the different registry based on aarch. or it is possible to
modify the sub to return an array in the future.

I tried to group some of the versions as they provide commonalities but regular
hash accept scalar as a key.

pdostal · 2021-08-27T09:12:42Z

lib/containers/urls.pm

-    $dotversion = "${dotversion}.0" if $dotversion !~ /\./;    # 15 -> 15.0
+    my $version = shift // get_required_var('VERSION');
+    my $arch    = shift // get_required_var('ARCH');
+    my $distri  = shift // get_required_var('DISTRI');

    my @untested_images = ();


Please also rename to totest so it's consistent.

untested_images as well as released_images came from a previous team agreement. i dont want to interfere and change them. i think it is messy to change the names frequently. in the other side i think the variable names in the structure are quite good and they do not mess with the already implementation.

I like your totest_ and released_ prefixes. But I'd like to use the same prefixes everywhere.

Maybe another point: I'd rename %registries to %images and released_registry to released_image and totest_registry to totest_image.

@jlausuch @Vogtinator can you help here so we agree on same convention and use it everywhere?

Ping. I would still be for unifying those names. E.g. released_images and totest_images as in urls.pm.

Let's just not forget about it.

My objection is that arbitrary change of the variable names or unrelated request of the scope is bad practice. The reason is because usually you trigger side effects and it might cause a chain of reactions+additional checks,VR and etc. Second, you request to rename some variables, unrelated to the scope of this PR, which as for beginning they are not bad ones and the discussion should be in the case the PR have made changes on them. Which it doesnt and a very important reason of it is that there was a PR which reviewed and approved (requester was one of them) after a long talk on the rocket.chat if i remember. i just think is really bad to approve something and someone changed it without a good "reason". this causes more issues and inconsistency in the reviews and waste our time.

tespa. i would like to leave it as it is for the reasons i just mentioned. and you can decide later what its destiny will be :)

I am in favor of renaming %registries to %images.

this is a definition think that i try to address here. the image name and the image registry is not exactly the same. The image registry is the uri of the image where is stored. When we execute podman run hello-world , the runtime will check the image-name in the registry and then at the docker registry from convention. in both cases it looks on localhost:5000/hello-world and docker.io/hello-world. this is the registry and the hash provides obviously the registry-name rather the image-name. i thought to force this convention as for me it is confusing when we do not use the same terminology.

this change is ready for review

Just call it hash :D .
Sorry for the joke, but this is just another "tab or spaces" discussion.. I don't care about the name.

My objection is that arbitrary change of the variable names or unrelated request of the scope is bad practice. The reason is because usually you trigger side effects and it might cause a chain of reactions+additional checks,VR and etc. Second, you request to rename some variables, unrelated to the scope of this PR, which as for beginning they are not bad ones and the discussion should be in the case the PR have made changes on them. Which it doesnt and a very important reason of it is that there was a PR which reviewed and approved (requester was one of them) after a long talk on the rocket.chat if i remember. i just think is really bad to approve something and someone changed it without a good "reason". this causes more issues and inconsistency in the reviews and waste our time.

tespa. i would like to leave it as it is for the reasons i just mentioned. and you can decide later what its destiny will be :)

I agree with this, it's not something for this PR as I mentioned.

lib/containers/urls.pm

pdostal · 2021-08-27T09:15:17Z

This so far looks great to me!

Vogtinator

"+202 -57", 145 more lines while also being less flexible :-/

Having a hash of subs means that it's not just plain data, but still code, IMO the shorter code was simpler, but that's just my view.

It's no longer possible to distinguish between "not available, skip" (e.g. 15.2 PowerPC) and "not known, abort" (none of the cases match).

The biggest issue IMO however is that it needs manual changes for every new version of SLE or Leap, while currently it handles later version of known distributions automatically.

Vogtinator · 2021-09-09T13:26:19Z

It's no longer possible to distinguish between "not available, skip" (e.g. 15.2 PowerPC) and "not known, abort" (none of the cases match).

Actually, that is possible. push @foo, bar(); is a noop if bar doesn't return anything. push @foo, undef; however isn't. Perl is weird.

jlausuch · 2021-09-09T14:29:19Z

"+202 -57", 145 more lines while also being less flexible :-/

Having a hash of subs means that it's not just plain data, but still code, IMO the shorter code was simpler, but that's just my view.

Yes, it's more code but it's also more clear. I am in favor of this format. Maybe, in the future we could put it in json/yaml/xml format in data directory and just read it here, so we don't create so many lines of code with that big hash. Something similar to what we do for journal_check test. But it's fine if that is done later.

I have approach this problem organizing the data into a hash. The data structure can hold information explicitly, so it is more ease to handle. Each <distri><version> hold three information - released - totest - available_arch I use anonumous functions for `registries` to provide flexibility. For instance you can choose the different registry based on aarch. or it is possible to modify the sub to return an array in the future. I tried to group some of the versions as they provide commonalities but regular hash accept scalar as a key. Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com>

Vogtinator

The biggest issue IMO however is that it needs manual changes for every new version of SLE or Leap, while currently it handles later version of known distributions automatically.

Is still valid

b10n1k · 2021-09-10T12:28:28Z

The biggest issue IMO however is that it needs manual changes for every new version of SLE or Leap, while currently it handles later version of known distributions automatically.

Is still valid

Sorry i thought that @jlausuch has answered on that and seems to be in favor of this structure. is this somehow different?

My opinion is that this is a more neat approach because you can "describe" what is your data. there is a trade off with the procedural logic. the writing/reading is getting harder because the decision made by mixing version, distri and arch. the task was to simplify this.

To avoid manual changes we could use this struct a little bit different.

our %registries = (
    sle => {
        '12' => {
            released => sub { 'registry.suse.com/suse/sles12sp3' },
            totest   => sub {
                'registry.suse.de/' . get_sle12($version);
            },
            available_arch => ['x86_64', 'ppc64le', 's390x']
        },
        15' => {
            released => sub { 'registry.suse.com/suse/sle15:15.0' },
            totest   => sub {
                'registry.suse.de/'' . get_sle15($version);
            },
            available_arch => ['x86_64', 'ppc64le', 's390x']
        },
    },
    opensuse => {
        Tumbleweed => {
            released => sub { 'registry.opensuse.org/opensuse/tumbleweed' },
            totest   => sub {
                'registry.opensuse.org/' . get_opensuse_registry_prefix . 'opensuse/tumbleweed';
            },
            available_arch => ['x86_64', 'aarch64', 'ppc64le', 's390x', 'arm']
        },
        'leap => {
            released => sub { 'registry.opensuse.org/opensuse/leap:15.0' },
            totest   => sub {
                'registry.opensuse.org/ . get_leap($version)';
            },
            available_arch => ['x86_64']
        }
       });

or something like that.
So for sure it will be nice to have it, it is possible on my point of view but i think there is time to figure this out.

lib/containers/urls.pm

ilausuch

For me is good, only that I agree to change the registries variable name to images, more clear what contains

jlausuch · 2021-09-13T07:09:06Z

The biggest issue IMO however is that it needs manual changes for every new version of SLE or Leap, while currently it handles later version of known distributions automatically.

Is still valid

Sorry i thought that @jlausuch has answered on that and seems to be in favor of this structure. is this somehow different?

Yes, I am in favor. If we need to add new releases it won't be a big deal, maybe once or twice a year? For new releases there are actually bigger changes we need to do, adding a few lines here is not that much of a problem.

The only thing against having this hash here is just that it takes too much lines in a .pm module, so I would be also in favor to transform this into json file and parse it here, but this can be done in another PR.

b10n1k · 2021-09-13T08:13:13Z

For me is good, only that I agree to change the registries variable name to images, more clear what contains

#13141 (comment). feel free to suggests something different. for instance %images_uri but i still think that registries describes it just fine.

pdostal · 2021-09-14T08:19:53Z

For me is good, only that I agree to change the registries variable name to images, more clear what contains

#13141 (comment). feel free to suggests something different. for instance %images_uri but i still think that registries describes it just fine.

Either %images_uri or just %images work for me. Thx Yianni

jlausuch · 2021-09-14T08:21:40Z

It seems this is being blocked because of the hash name. Let's just let people vote:
%registries or %images.

I vote for %images

ilausuch · 2021-09-14T08:25:38Z

I vote for %images, %registries confuses for me

b10n1k · 2021-09-14T08:48:53Z

It seems this is being blocked because of the hash name. Let's just let people vote:
%registries or %images.

I vote for %images

are we going to vote for each review conflict? i even proposed another name... and i will add a few here in case you want to pick something from that which will define the hash uniquely

%images_uri
%images_struct
%registry_images
%suse_images

Again consider that the hash is not a private variable and it is worth to give a better name than %images

jlausuch · 2021-09-14T10:03:22Z

* %images_uri
* %images_struct
* %registry_images
* %suse_images

%images_uri +1

`get_suse_container_urls` takes named params now as requested. With this commit, apply this to the files that has explicit impact, as they use positional one Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com>

b10n1k · 2021-09-14T13:00:20Z

* %images_uri
* %images_struct
* %registry_images
* %suse_images

%images_uri +1

First comes First Served?? :) this change is ready for review

pdostal · 2021-09-15T06:35:15Z

The biggest issue IMO however is that it needs manual changes for every new version of SLE or Leap, while currently it handles later version of known distributions automatically.

Is still valid

This is in my opinion out of scope of this PR and also quite complex problem as we test prerelease versions too.

b10n1k · 2021-09-15T08:28:34Z

follow up ticket to address manual changes to the hash https://progress.opensuse.org/issues/98658

DimStar77 · 2021-09-15T20:37:37Z

This seems responsible for the staging failures, like
https://openqa.opensuse.org/tests/1918468#step/podman_image/28

pdostal reviewed Aug 27, 2021

View reviewed changes

lib/containers/urls.pm Outdated Show resolved Hide resolved

b10n1k force-pushed the 97232_containers_urls branch 27 times, most recently from dcd2b30 to c65ae8f Compare August 30, 2021 21:02

Vogtinator requested changes Sep 9, 2021

View reviewed changes

michaelgrifalconi and others added 2 commits September 9, 2021 21:07

Revert "Import gpg untrusted key changed to yes/no"

fc7a423

b10n1k force-pushed the 97232_containers_urls branch from 17ed2bc to f18bab5 Compare September 10, 2021 08:18

jlausuch approved these changes Sep 10, 2021

View reviewed changes

b10n1k requested a review from Vogtinator September 10, 2021 09:19

Vogtinator requested changes Sep 10, 2021

View reviewed changes

pdostal reviewed Sep 12, 2021

View reviewed changes

lib/containers/urls.pm Outdated Show resolved Hide resolved

ilausuch approved these changes Sep 13, 2021

View reviewed changes

b10n1k requested a review from Vogtinator September 14, 2021 10:22

Apply side-effect changes by get_suse_container_urls signature

ecb65e0

`get_suse_container_urls` takes named params now as requested. With this commit, apply this to the files that has explicit impact, as they use positional one Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com>

b10n1k force-pushed the 97232_containers_urls branch from 72cee52 to ecb65e0 Compare September 14, 2021 11:53

pdostal approved these changes Sep 15, 2021

View reviewed changes

b10n1k merged commit 27644d8 into os-autoinst:master Sep 15, 2021

Vogtinator mentioned this pull request Sep 16, 2021

Revert "Refactor get_suse_container_urls using data structure for each distri" #13266

Merged

b10n1k deleted the 97232_containers_urls branch August 24, 2023 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor get_suse_container_urls using data structure for each distri #13141

Refactor get_suse_container_urls using data structure for each distri #13141

b10n1k commented Aug 27, 2021 •

edited

Loading

pdostal Aug 27, 2021

b10n1k Aug 31, 2021 •

edited

Loading

pdostal Aug 31, 2021

pdostal Aug 31, 2021

pdostal Sep 3, 2021 •

edited

Loading

pdostal Sep 8, 2021

b10n1k Sep 9, 2021

b10n1k Sep 9, 2021 •

edited

Loading

jlausuch Sep 9, 2021 •

edited

Loading

jlausuch Sep 9, 2021

pdostal commented Aug 27, 2021

Vogtinator left a comment

Vogtinator commented Sep 9, 2021

jlausuch commented Sep 9, 2021

Vogtinator left a comment

b10n1k commented Sep 10, 2021 •

edited

Loading

ilausuch left a comment

jlausuch commented Sep 13, 2021

b10n1k commented Sep 13, 2021

pdostal commented Sep 14, 2021

jlausuch commented Sep 14, 2021

ilausuch commented Sep 14, 2021

b10n1k commented Sep 14, 2021

jlausuch commented Sep 14, 2021

b10n1k commented Sep 14, 2021

pdostal commented Sep 15, 2021

b10n1k commented Sep 15, 2021

DimStar77 commented Sep 15, 2021

Refactor get_suse_container_urls using data structure for each distri #13141

Refactor get_suse_container_urls using data structure for each distri #13141

Conversation

b10n1k commented Aug 27, 2021 • edited Loading

pdostal Aug 27, 2021

Choose a reason for hiding this comment

b10n1k Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

pdostal Aug 31, 2021

Choose a reason for hiding this comment

pdostal Aug 31, 2021

Choose a reason for hiding this comment

pdostal Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

pdostal Sep 8, 2021

Choose a reason for hiding this comment

b10n1k Sep 9, 2021

Choose a reason for hiding this comment

b10n1k Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

jlausuch Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

jlausuch Sep 9, 2021

Choose a reason for hiding this comment

pdostal commented Aug 27, 2021

Vogtinator left a comment

Choose a reason for hiding this comment

Vogtinator commented Sep 9, 2021

jlausuch commented Sep 9, 2021

Vogtinator left a comment

Choose a reason for hiding this comment

b10n1k commented Sep 10, 2021 • edited Loading

ilausuch left a comment

Choose a reason for hiding this comment

jlausuch commented Sep 13, 2021

b10n1k commented Sep 13, 2021

pdostal commented Sep 14, 2021

jlausuch commented Sep 14, 2021

ilausuch commented Sep 14, 2021

b10n1k commented Sep 14, 2021

jlausuch commented Sep 14, 2021

b10n1k commented Sep 14, 2021

pdostal commented Sep 15, 2021

b10n1k commented Sep 15, 2021

DimStar77 commented Sep 15, 2021

b10n1k commented Aug 27, 2021 •

edited

Loading

b10n1k Aug 31, 2021 •

edited

Loading

pdostal Sep 3, 2021 •

edited

Loading

b10n1k Sep 9, 2021 •

edited

Loading

jlausuch Sep 9, 2021 •

edited

Loading

b10n1k commented Sep 10, 2021 •

edited

Loading