Introduce new method get_latest_build #58

asmorodskyi · 2024-12-09T17:22:05Z

One of many common tasks which you need to address when interacting
with openQA programatically is to detect what was latest build in certain
Job Group. This is implementation of such function in this lib, so
users of this lib may delete same logic duplicated many times

src/openqa_client/client.py

okurz

minor typo in git commit message s/interucting/interacting/, rest is fine

AdamWill · 2024-12-11T18:17:32Z

This seems somewhat tied to SUSE's conception of builds. In Fedora, for instance, builds are never really sortable, or only sortable within very specific constraints. This is why the webUI has the option to sort builds by the time they were created, which we always use in Fedora.

If we're going to merge this it might be nice if it could similarly sort the builds by creation time, though I'm not sure off the top of my head whether the API exposes that information conveniently.

asmorodskyi · 2024-12-11T21:08:46Z

This seems somewhat tied to SUSE's conception of builds. In Fedora, for instance, builds are never really sortable, or only sortable within very specific constraints. This is why the webUI has the option to sort builds by the time they were created, which we always use in Fedora.

actual name of configuration option which you mean here is Sort by time job most recently created . Which in a fact means that sorting will happen NOT "by the time they were created" but by the time when last job in this certain build was executed . IMHO this make definition of what to call "Latest build" a bit blurry . If I triggered build "X" one week ago and build "Y" today but also today I restarted the job for build "X" what would you expect to be called latest one ?

If we're going to merge this it might be nice if it could similarly sort the builds by creation time, though I'm not sure off the top of my head whether the API exposes that information conveniently.

https://openqa.opensuse.org/api/v1/job_groups/1/build_results has "oldest" field which would define oldest job linked to this buid but sorting by this field would be incosistent because restarting jobs would give you different results . And I was not able to find something else unfortunately.

asmorodskyi · 2024-12-11T21:14:13Z

minor typo in git commit message s/interucting/interacting/, rest is fine

fixed in both actual commit message and here in PR description

AdamWill · 2024-12-12T00:09:24Z

actual name of configuration option which you mean here is Sort by time job most recently created . Which in a fact means that sorting will happen NOT "by the time they were created" but by the time when last job in this certain build was executed

Oh, yes, that's right, I forgot the details. Honestly, sorting by the time the build first showed up would be better for us, both in the web UI and here. But it may require work on upstream openQA, I'm not sure.

IMHO this make definition of what to call "Latest build" a bit blurry . If I triggered build "X" one week ago and build "Y" today but also today I restarted the job for build "X" what would you expect to be called latest one ?

Yes, it does have this drawback, but it's much less bad than sorting "build names" like the ones in https://openqa.fedoraproject.org/group_overview/2 alphanumerically :D In practice, it is indeed the build for which a job was most recently created that comes on top of the list, i.e. it'd be X if you restarted the X job after you triggered Y.

asmorodskyi · 2024-12-12T07:51:02Z

Yes, it does have this drawback, but it's much less bad than sorting "build names" like the ones in https://openqa.fedoraproject.org/group_overview/2 alphanumerically :D In practice, it is indeed the build for which a job was most recently created that comes on top of the list, i.e. it'd be X if you restarted the X job after you triggered Y.

sure I can do that . but I see two options :

Add flag to current function which I am introducing . This IMO will make code a bit messy with too many input params and also quite some if/else inside
Rename current function into get_latest_build_by_name and also add get_latest_build_by_time where this two totally different approaches will be divided and each one can decide about its own flavor of latest

@okurz , @Martchus , @AdamWill WDYT ?

asmorodskyi · 2024-12-12T08:19:31Z

Oh, yes, that's right, I forgot the details. Honestly, sorting by the time the build first showed up would be better for us, both in the web UI and here. But it may require work on upstream openQA, I'm not sure.

fully agree here , sorting should be done not by oldest job but by youngest this would make more sense . And yes before we can follow this approach here some work needs to be done in openQA backend ...

Martchus · 2024-12-12T09:59:24Z

I think we actually have a ticket about improving the time-based sorting in openQA to consider the first jobs in that build (so re-triggering jobs will not change the order). It was not clear whether this is problematic because it was not clear whether it conflicts with @AdamWill 's use case so we even considered making this a 3rd option. However, it actually seems to support @AdamWill 's use case so we could change the existing time-based sorting and avoid adding a 3rd option. Unfortunately I cannot find that ticket at the moment.

okurz · 2024-12-12T10:00:18Z

that would be https://progress.opensuse.org/issues/57335

okurz · 2024-12-12T10:03:44Z

@AdamWill if you don't see further issues blocking feel welcome to merge

AdamWill · 2024-12-12T16:30:12Z

tests/test_client.py

+    def test_get_latest_build(self, fakerequest):
+        client = oqc.OpenQA_Client()
+        with pytest.raises(TypeError):
+            client.get_latest_build()


why this? are we just checking how Python behaves when a call is missing a required argument? do we really...need to?

TBH it does not make sense to me too , but I was just trying to following same logic used by other tests here . Should I also delete same pattern in lines 368-369 ?

with pytest.raises(TypeError): client.get_jobs()

We have that there because get_jobs is kinda 'special'. All its arguments are optional, syntactically speaking. Python itself won't raise TypeError if you do get_jobs(). But we actually need you to pass either the jobs arg or the build arg, and if you don't pass either of those, we (that is, openQA-python-client) raise TypeError - that's line 364 in client.py. Because that code is in our library it needs test coverage.

I actually hate this and wish I hadn't built it that way, but we're stuck with it now. :P

so yeah, leave test_get_jobs alone, but remove the check from this PR. It's not needed here, since it's not our code that raises TypeError here, so we don't need to test it. We're just relying on completely standard Python behaviour.

er...but...you didn't remove the check?

I just first wrote then did the push , sorry . NOW it is done

tests/test_client.py

AdamWill · 2024-12-12T18:14:30Z

src/openqa_client/client.py

+            group_id (int): Job Group ID
+            all_passed (bool, optional): Controls whether just last build will be selected
+                or the last with all jobs passed. Defaults to True.
+            sorted_key (Callable, optional): To find the latest build we need to order the builds.


I don't really like the name sorted_key, the past tense is odd. I'd call it sort_key. The description is a bit odd, it doesn't really explain that this is specifically the key used to sort the results.

The description is a bit odd, it doesn't really explain that this is specifically the key used to sort the results.

The specified callback will be used in the sorted function to sort builds 🤔

maybe just The specified callable will be passed to sorted() to sort the builds. ?

it doesn't look like it's done? i still see the original text, "sorted_key (Callable, optional): To find the latest build we need to order the builds."

you are too fast :) I am tweaking code in my working copy and marking conversation done so I know that I solved this one . After I go through ALL long list of endless discussions in this PR I am doing the push . This create a gap in 20 minutes between I wrote "done" and you can actually can confirm that . I was not expecting that you will get to the PR so fast , sorry . NOW it is done

src/openqa_client/client.py

asmorodskyi · 2024-12-13T23:28:18Z

@AdamWill I would kindly ask you to press "Resolve conversation" in discussions where you find problem solved so I can follow up on what is left

src/openqa_client/client.py

foursixnine · 2024-12-14T17:51:37Z

just because this called my attention today when goigh through my gh notifications, doesn't the webUI already implements that? i.e see the test distribution and how it uses it ( nvm, just realized that it still depends on very smelly jq 😭 https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/.github/workflows/openqa.yml#L32 )'

perlpunk · 2024-12-16T15:22:13Z

( nvm, just realized that it still depends on very smelly jq 😭 https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/.github/workflows/openqa.yml#L32 )'

https://progress.opensuse.org/issues/152939

src/openqa_client/client.py

One of many common tasks which you need to address when interacting with openQA programatically is to detect what was latest build in certain Job Group. This is implementation of such function in this lib, so users of this lib may delete same logic duplicated many times

foursixnine · 2024-12-17T12:15:47Z

( nvm, just realized that it still depends on very smelly jq 😭 https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/.github/workflows/openqa.yml#L32 )'

https://progress.opensuse.org/issues/152939

ahhh right, that was the thing, for TW the latest published build was the thing that the jq par does.

Martchus requested changes Dec 10, 2024

View reviewed changes

asmorodskyi force-pushed the latest_build branch from f5717f0 to 1c94d7a Compare December 10, 2024 14:36

Martchus approved these changes Dec 10, 2024

View reviewed changes

okurz requested changes Dec 11, 2024

View reviewed changes

asmorodskyi force-pushed the latest_build branch from 1c94d7a to 43ae687 Compare December 11, 2024 21:13

okurz approved these changes Dec 12, 2024

View reviewed changes

AdamWill requested changes Dec 12, 2024

View reviewed changes

asmorodskyi force-pushed the latest_build branch 3 times, most recently from 5fb2941 to 0a1521c Compare December 13, 2024 23:25

AdamWill reviewed Dec 14, 2024

View reviewed changes

src/openqa_client/client.py Outdated Show resolved Hide resolved

asmorodskyi force-pushed the latest_build branch from 0a1521c to 0eec4bd Compare December 15, 2024 20:44

perlpunk reviewed Dec 16, 2024

View reviewed changes

src/openqa_client/client.py Outdated Show resolved Hide resolved

asmorodskyi force-pushed the latest_build branch from 0eec4bd to 1e9e27e Compare December 17, 2024 07:31

asmorodskyi force-pushed the latest_build branch from 1e9e27e to 1f635b6 Compare December 17, 2024 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new method get_latest_build #58

Introduce new method get_latest_build #58

asmorodskyi commented Dec 9, 2024 •

edited

Loading

okurz left a comment

AdamWill commented Dec 11, 2024 •

edited

Loading

asmorodskyi commented Dec 11, 2024 •

edited

Loading

asmorodskyi commented Dec 11, 2024

AdamWill commented Dec 12, 2024

asmorodskyi commented Dec 12, 2024

asmorodskyi commented Dec 12, 2024

Martchus commented Dec 12, 2024

okurz commented Dec 12, 2024

okurz commented Dec 12, 2024

AdamWill Dec 12, 2024

asmorodskyi Dec 12, 2024

AdamWill Dec 16, 2024 •

edited

Loading

asmorodskyi Dec 17, 2024

AdamWill Dec 17, 2024

asmorodskyi Dec 17, 2024

AdamWill Dec 12, 2024

asmorodskyi Dec 12, 2024

asmorodskyi Dec 12, 2024

AdamWill Dec 16, 2024

asmorodskyi Dec 17, 2024

AdamWill Dec 17, 2024

asmorodskyi Dec 17, 2024

asmorodskyi commented Dec 13, 2024

foursixnine commented Dec 14, 2024 •

edited

Loading

perlpunk commented Dec 16, 2024

foursixnine commented Dec 17, 2024

Introduce new method get_latest_build #58

Are you sure you want to change the base?

Introduce new method get_latest_build #58

Conversation

asmorodskyi commented Dec 9, 2024 • edited Loading

okurz left a comment

Choose a reason for hiding this comment

AdamWill commented Dec 11, 2024 • edited Loading

asmorodskyi commented Dec 11, 2024 • edited Loading

asmorodskyi commented Dec 11, 2024

AdamWill commented Dec 12, 2024

asmorodskyi commented Dec 12, 2024

asmorodskyi commented Dec 12, 2024

Martchus commented Dec 12, 2024

okurz commented Dec 12, 2024

okurz commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamWill Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmorodskyi commented Dec 13, 2024

foursixnine commented Dec 14, 2024 • edited Loading

perlpunk commented Dec 16, 2024

foursixnine commented Dec 17, 2024

asmorodskyi commented Dec 9, 2024 •

edited

Loading

AdamWill commented Dec 11, 2024 •

edited

Loading

asmorodskyi commented Dec 11, 2024 •

edited

Loading

AdamWill Dec 16, 2024 •

edited

Loading

foursixnine commented Dec 14, 2024 •

edited

Loading