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

Add async OCI and OCM packages #1053

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Add async OCI and OCM packages #1053

merged 2 commits into from
Oct 11, 2024

Conversation

8R0WNI3
Copy link
Member

@8R0WNI3 8R0WNI3 commented Oct 2, 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-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 Oct 2, 2024
@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Oct 2, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Oct 2, 2024
@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 Oct 2, 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 Oct 2, 2024
@8R0WNI3 8R0WNI3 added the area/ipcei IPCEI (Important Project of Common European Interest) label Oct 7, 2024
@ccwienk

This comment was marked as resolved.

@ccwienk
Copy link
Member

ccwienk commented Oct 7, 2024

as for the "actual" changes, I have some concerns w.r.t. new package-names. considering other patterns I have seen, it seem to be more common to have (few) different entrypoints (e.g. client.Client vs client.AsyncClient) or use async-suffixes (e.g. client + client_async).

Specifically for oci-package (or more specifically oci/__init__.py, I think we either might drop some parts (e.g. kaniko-related code should not be actually in-use any longer, so we might discard an async flavour of it). From the diffs I thus-far inspected, I can see there is quite some duplication w.r.t. dataclasses - I think we should deduplicate those (there should not be any differences between them (sync vs async), should there?.

How about a layout like the following (limited to oci package for now):

oci/__init__.py # functions are replicated w/ _async-suffix: replicate_artifact_async, replicate_blobs_async, drop kaniko (at least: do not re-implement/duplicate as async)
oci/client,py # Client + AsyncClient (same module); as an alternative: client + client_base + client_async (factor-out re-usable code (login/token-handling, routes); this has the slight disadvantage of not being fully backwards-compatible (although we might fix that by exporting imports from client_base in client-module)
oci/platform.py # check which functions we actually use; create async versions of needed ones (in same module)
oci/workarounds.py # ditto

@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 Oct 7, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 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 Oct 7, 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 Oct 7, 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 Oct 7, 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 Oct 7, 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 Oct 7, 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 Oct 7, 2024
oci/__init__.py Outdated Show resolved Hide resolved
oci/__init__.py Outdated Show resolved Hide resolved
oci/__init__.py Outdated Show resolved Hide resolved
oci/client.py Outdated Show resolved Hide resolved
oci/client.py Outdated Show resolved Hide resolved
cnudie/iter.py Outdated Show resolved Hide resolved
cnudie/retrieve.py Outdated Show resolved Hide resolved
@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 Oct 9, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 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 Oct 9, 2024
@8R0WNI3 8R0WNI3 requested a review from ccwienk October 9, 2024 08:04
oci/__init__.py Outdated Show resolved Hide resolved
oci/client.py Outdated Show resolved Hide resolved
requirements.oci.txt Outdated Show resolved Hide resolved
@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 Oct 10, 2024
@8R0WNI3 8R0WNI3 requested a review from ccwienk October 10, 2024 07:01
@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 Oct 10, 2024
requirements.txt Outdated Show resolved Hide resolved
Copy link
Member

@ccwienk ccwienk left a comment

Choose a reason for hiding this comment

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

lgtm - please rm aiohttp from "hard" dependencies, as suggested by @zkdev

@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 Oct 11, 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 Oct 11, 2024
@8R0WNI3 8R0WNI3 merged commit a0cd371 into master Oct 11, 2024
12 checks passed
@8R0WNI3 8R0WNI3 deleted the 8R0WNI3-async branch October 11, 2024 10:11
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipcei IPCEI (Important Project of Common European Interest) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (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.

7 participants