improve dora implementation #241

Finnegan1 · 2024-11-11T00:25:33Z

Special notes for your reviewer:
The return json object is not compitable with the version before.
So only accept together with the according delivery dashboard PR.

ocm-ci-robot-0 · 2024-11-11T00:26:04Z

Thank you @Finnegan1 for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

components.py

dora.py

zkdev · 2024-11-11T09:18:30Z

dora.py

-            ]
-        ))
+        ):
+            print("can't process")


please elaborate on what's wrong

In this case it checks if the repository is a github repo. This happens because currently we can only access Github repos. If it is not an Github repo, we just skip it :/

@Finnegan1 : skipping unsupported resources is fine. With my (quite brief) comment, I wanted to suggest to make the logged message more expressive, and self-describing. Consider you look through the log and just see "can't process". This will make it very difficult to understand (without knowing the context / emitting code) what happened / whether or not it is a problem.

A better log-message might read: skipping {component-name+version}/{artefact-name}: has unsupported {type=} (we only support gitHub.

In addition, you should preferably use logging package (I think in this specific case, debug might be an adequate severity/loglevel)

zkdev · 2024-11-11T09:18:39Z

dora.py

-            ))
-        entry_date += datetime.timedelta(days=30)
+        if not old_repo_url == new_repo_url:
+            print("repo urls are not equal")


I deleted the print :)

This is another edge case. If the repository of the component chaned between two updates, It can not compare it with the healp of the hashes. So for now it just skips this update.

Should I log this?

Thank you for the explanation.
I think logging is useful here, probably even as warning because a component repository change is quite uncommon.

@Finnegan1 : it would be very helpful do document such edge-cases, ideally in a (brief) comment close to the code doing the handling.

However, I would like to remark that a change or repo-url does not necessarily mean the repository itself changed. The repository might simply have been moved (we, for example, recently moved this repository to its current location; although admittedly, in this specific case, we also changed the repository's commit-history)

OTOH, just because repository-URL did not change does not necessarily mean all commits stored in component-descriptors are actually still in (consider a forceful history-rewrite, that might be done upon publishing an unintended commit (e.g. containing credentials, or a large file that was added by accident)).

So instead of this early-exit, I would rather suggest to change the code such that the case not both commits are found in remote-repository should be handled

app.py

8R0WNI3 · 2024-11-14T06:33:17Z

components.py

@@ -76,6 +76,16 @@ async def check_if_component_exists(
    return False


+def ensure_utc(ts: datetime.datetime) -> datetime.datetime:
+    if ts.tzinfo is None:


Is there a reason why explicitly checking for None here is required? If not, I'd suggest to simply check if ts.tzinfo is falsy or not.

components.py

8R0WNI3 · 2024-11-14T06:44:33Z

dora_result_calcs.py

+    leadTime: datetime.timedelta
+    url: str
+
+    def to_dict(self):


Instead of writing all these to_dict functions in this module, you can also just utilise util.dict_serialisation which will already recursively serialise your dataclasses into a dict. Note however, that this will require an additional condition for handling datetime.timedelta objects. As an alternative, you might also use built-in dataclasses.asdict functionality and pass-in a a custom dict_factory to specify serialisation of datetime.datetime and datetime.timedelta objects.

Should I just let the ones which do already exist as they are or should I migrate them to util.dict_serialisation?

dora_result_calcs.py

dora.py

8R0WNI3 · 2024-11-14T08:42:25Z

dora_result_calcs.py

@@ -0,0 +1,140 @@
+import collections


Let us rather merge this module into existing dora.py.

I will let it seperated for now, so that you can see the changes better. I will move it to dora.py as the last step 👍

ccwienk · 2024-11-14T11:12:54Z

components.py

@@ -76,6 +76,16 @@ async def check_if_component_exists(
    return False


+def ensure_utc(ts: datetime.datetime) -> datetime.datetime:


If I am not mistaken, this function will:

return timestamp unchanged if it is tz-aware + tz equals utc

return cp of ts w/ tz converted to utc it tz-aware + tz != utc

return a cp of passed-in ts w/ tz to utc if ts is not tz-aware

Hence, I think the name is misleading + I am uncertain about why conversion (of tz-aware TSs) to utc is needed in the first place.

-> I imagine it should be sufficient for this function leave tz-aware TSs as they are + set tz for tz-unaware TSs. If this is not the case, could you elaborate on why?

Assuming conversion to utc for tz-aware TSs is not necessary, a better name might be as_tz_aware, or make_tz_aware, or similar. Also, it might be considered to expose target-tz as parameter (it is okay to prefill it w/ utc to make it more convenient for callers).

It should be noted, though, that "blindly" assuming utc as timezone will yield an error of up to 23h (or is it 24? 🤔 ). So the preferred approach IMO might be to actually go through the efforts and ensure each location in our code, where we create timestamps does so in a tz-aware manner.

naming change is a good idea

good idea to make the "wanted" time zone as a parameter.

For the reason why this function exists in the first place:
=> This function is only used when handeling external data => timestamps from Github commits and creation dates of Component descriptors.
=> To ensure, that there are no false results when calculating with the "internal" dates which are (at least should it be so) all UTC

timestamps from component-descriptors should actually be tz-aware (I think this is even forced by the ocm-spec); there might be historical component-descriptors w/ non-tz-aware timestamps though (do you happen to remember whether you actually encountered non-tz-aware TSs from component-descriptors? If not, it might be cleaner to rm this codepath for TSs from component-descriptors

As for the timestamps contained in git-commits, I think those should also be tz-aware (again: do you have concrete examples of commits in git-repositories which are not tz-aware?) 🤔

As for conversion to UTC: If I am not mistaken, python's datetime-API will make no mistakes when calculating timeranges and the like using tz-aware timestamps.

Soooo in summary:

please check whether we actually have non-tz-aware TSs

conversion of tz-aware TSs is only necessary to make timestamps "human-friendly" (for displaying/logging), but it is not needed as preliminary step for date-calculations

-> it might be possible to drop this function - unless there are non-tz-aware TSs, of course

dora.py

ccwienk · 2024-11-14T11:31:06Z

dora.py

-            ]
-        ))
+        ):
+            print("can't process")


please elaborate on what's wrong

ccwienk · 2024-11-14T11:32:34Z

dora.py


+@middleware.auth.noauth


maybe to give some context: we recently switched all routes (except the one for doing auth, for obvious reasons :-)) to requiring authentication

dora.py

ccwienk · 2024-11-14T11:41:18Z

please address linter + test errors - ty

ccwienk · 2024-12-02T09:27:52Z

@Finnegan1 : I added some additional remarks to ongoing discussions. However, those were not attached to the (changed) code - please browse through discussions in "Conversation" Tab

ccwienk · 2024-12-03T11:07:59Z

dora_result_calcs.py

@@ -73,38 +74,65 @@ def to_dict(self):
            'deployents': [deployment.to_dict() for deployment in self.deployments],
        }

+@dataclasses.dataclass(frozen=True)


considering attrs are dicts (which are mutable), the dataclass should not be declared frozen (immutable)

ccwienk · 2024-12-03T11:08:23Z

dora_result_calcs.py

@@ -73,38 +74,65 @@ def to_dict(self):
            'deployents': [deployment.to_dict() for deployment in self.deployments],
        }

+@dataclasses.dataclass(frozen=True)
+class DeploymentFrequencies:
+    deploymentsPerMonth: dict[typing.Any, int]


shouldn't key's typehint be str, rather than Any?

ccwienk · 2024-12-03T11:09:23Z

dora_result_calcs.py

-        'medianLeadTimePerDay': median_lead_time_per_day,
-    }
+    return LeadTimes(
+        medianLeadTimePerMonth=dict(median_lead_time_per_month),


aren't those values already dicts?

ccwienk · 2024-12-03T11:09:54Z

util.py

+
+
+def as_timezone(ts: datetime.datetime, target_tz: datetime.timezone = datetime.timezone.utc) -> datetime.datetime:
+    if ts.tzinfo is None:


as discussed in separate thread: is tz-conversion actually needed?

…current

improve dora implementation

38a8edf

Finnegan1 requested a review from a team as a code owner November 11, 2024 00:25

zkdev added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 11, 2024

ocm-ci-robot-0 added needs/ok-to-test and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 11, 2024

zkdev requested changes Nov 11, 2024

View reviewed changes

8R0WNI3 requested changes Nov 14, 2024

View reviewed changes

ccwienk requested changes Nov 14, 2024

View reviewed changes

first of the requested changes

c5f8d8d

ccwienk requested changes Dec 3, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into dore-improvement-…

6ee8643

…current

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve dora implementation #241

improve dora implementation #241

Finnegan1 commented Nov 11, 2024

ocm-ci-robot-0 commented Nov 11, 2024

zkdev Nov 11, 2024

ccwienk Nov 14, 2024

Finnegan1 Dec 1, 2024

ccwienk Dec 2, 2024

zkdev Nov 11, 2024

Finnegan1 Dec 1, 2024

zkdev Dec 2, 2024

ccwienk Dec 2, 2024

8R0WNI3 Nov 14, 2024

8R0WNI3 Nov 14, 2024

Finnegan1 Dec 1, 2024

8R0WNI3 Nov 14, 2024

Finnegan1 Dec 1, 2024

ccwienk Nov 14, 2024

Finnegan1 Dec 1, 2024

ccwienk Dec 2, 2024

ccwienk Nov 14, 2024

ccwienk Nov 14, 2024

ccwienk commented Nov 14, 2024 •

edited

Loading

ccwienk commented Dec 2, 2024

ccwienk Dec 3, 2024

ccwienk Dec 3, 2024

ccwienk Dec 3, 2024

ccwienk Dec 3, 2024

		@@ -76,6 +76,16 @@ async def check_if_component_exists(
		return False


		def ensure_utc(ts: datetime.datetime) -> datetime.datetime:



		def as_timezone(ts: datetime.datetime, target_tz: datetime.timezone = datetime.timezone.utc) -> datetime.datetime:
		if ts.tzinfo is None:


		@middleware.auth.noauth

improve dora implementation #241

Are you sure you want to change the base?

improve dora implementation #241

Conversation

Finnegan1 commented Nov 11, 2024

ocm-ci-robot-0 commented Nov 11, 2024

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

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

ccwienk commented Nov 14, 2024 • edited Loading

ccwienk commented Dec 2, 2024

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

ccwienk commented Nov 14, 2024 •

edited

Loading