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

Refactor ArtefactMetadata model classes #971

Merged
merged 7 commits into from
May 15, 2024

Conversation

8R0WNI3
Copy link
Collaborator

@8R0WNI3 8R0WNI3 commented May 13, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:


@gardener-robot-ci-1 gardener-robot-ci-1 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 May 13, 2024
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels May 13, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 May 13, 2024
dso/model.py Outdated Show resolved Hide resolved
@8R0WNI3 8R0WNI3 force-pushed the I538859-refactor-artefact-metadata-models branch from 4e7c5ff to 2bd3fbe Compare May 13, 2024 10:26
@gardener-robot-ci-1 gardener-robot-ci-1 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 May 13, 2024
@8R0WNI3 8R0WNI3 requested a review from zkdev May 13, 2024 10:26
@gardener-robot-ci-1 gardener-robot-ci-1 removed 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 May 13, 2024
Copy link
Member

@zkdev zkdev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels May 13, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 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 May 13, 2024
@8R0WNI3 8R0WNI3 force-pushed the I538859-refactor-artefact-metadata-models branch from 2bd3fbe to 43c9fcd Compare May 14, 2024 05:59
@gardener-robot-ci-1 gardener-robot-ci-1 removed 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 May 14, 2024
delivery/client.py Show resolved Hide resolved
delivery/client.py Show resolved Hide resolved
) -> str:
'''
generate stable representation of `artefact_extra_id` and remove `version` key if
the specified version is identical to the given artefact version
Copy link
Member

Choose a reason for hiding this comment

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

why special-handling for version? how about adding this to gci.componentmodel instead, like so:
-> id_tuple: returns (alphabetically) ordered tuple of (, )-pairs, which will always contain as many id-attrs as are available (always name, version if available, every attr from extra-id)?

this function's output might be used to also normalise into a str (we might use tuple's str for that or invent a custom format, as you suggested). however, as separation character, I would suggest to use one that is forbidden within keys/values - did you check ocm-spec whether this is true for underscore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We added the special-handling for the version already a while ago (ref). The reason behind this was that some artefacts contained their version also in the extra identity field, however, not always stable across different OCI repositories. Because of that, the comparison of the extra identities failed sometimes, depending of which OCI repository was used. To allow a stable representation of the extra identity independent of the OCI repository, we decided to allow the removal of the version attribute from the extra identity if desired.

Also, I just moved the normalise_artefact_extra_id function from delivery.model to dso.model, so I did not look into it in more detail, e.g. the separating characters. Nevertheless, I figure the underscore character is very basic in Python variable names and thus not forbidden within keys/values.

@8R0WNI3 8R0WNI3 requested a review from ccwienk May 14, 2024 13:22
@8R0WNI3 8R0WNI3 force-pushed the I538859-refactor-artefact-metadata-models branch from 43c9fcd to bada4e3 Compare May 14, 2024 13:22
@gardener-robot-ci-2 gardener-robot-ci-2 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 May 14, 2024
@gardener-robot gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) reviewed/lgtm Has approval for merging labels May 14, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed 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 May 14, 2024
@8R0WNI3 8R0WNI3 force-pushed the I538859-refactor-artefact-metadata-models branch from bada4e3 to 1c8d4ed Compare May 14, 2024 13:28
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 May 14, 2024
@8R0WNI3 8R0WNI3 force-pushed the I538859-refactor-artefact-metadata-models branch from 1c8d4ed to 65bbfd5 Compare May 14, 2024 13:39
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 14, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed 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 May 14, 2024
@8R0WNI3 8R0WNI3 merged commit f8db23d into master May 15, 2024
4 checks passed
@8R0WNI3 8R0WNI3 deleted the I538859-refactor-artefact-metadata-models branch May 15, 2024 08:46
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants