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

TODO and relative import cleanup #24

Merged
merged 7 commits into from
Oct 30, 2024

Conversation

isinyaaa
Copy link
Contributor

No description provided.

Signed-off-by: Isabella do Amaral <[email protected]>
Signed-off-by: Isabella do Amaral <[email protected]>
Signed-off-by: Isabella do Amaral <[email protected]>
omlmd/helpers.py Outdated Show resolved Hide resolved
omlmd/helpers.py Outdated Show resolved Hide resolved
omlmd/helpers.py Outdated Show resolved Hide resolved
Signed-off-by: Isabella do Amaral <[email protected]>
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

thanks a lot @isinyaaa !

some considerations below

omlmd/helpers.py Outdated Show resolved Hide resolved
omlmd/helpers.py Outdated Show resolved Hide resolved
omlmd/listener.py Outdated Show resolved Hide resolved
Signed-off-by: Isabella do Amaral <[email protected]>
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

thanks again; to me, let's defer the dataclass change of the Helper, at least for now. Added couple comments.

omlmd/helpers.py Outdated Show resolved Hide resolved
omlmd/helpers.py Show resolved Hide resolved
omlmd/helpers.py Outdated Show resolved Hide resolved
Comment on lines 39 to 40
def get_digest(self) -> str:
return self.response.headers["Docker-Content-Digest"] if self.ok else ""
Copy link
Contributor Author

@isinyaaa isinyaaa Oct 25, 2024

Choose a reason for hiding this comment

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

I like the idea of a helper to find the digest, but I'm not sure what this should do when the response is not ok.

Also, are we looking at a 200-ish or strictly 200 here?

Copy link
Member

Choose a reason for hiding this comment

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

  • let's return ValueError
  • 2xx please

Copy link
Contributor Author

@isinyaaa isinyaaa Oct 28, 2024

Choose a reason for hiding this comment

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

Oras-Py is already checking that for us before returning on push. As we can see
_check_200_response already performs the checks you specified.

Edit: updated accordingly. Let me know what you think.

@isinyaaa isinyaaa force-pushed the push-nlxwsytwopkq branch 3 times, most recently from 556d109 to d902e95 Compare October 25, 2024 13:47
@tarilabs
Copy link
Member

just fwiw, per PR original scope and per other conversations about modelcard, I don't think spending effort in 50ac4e5, 6a49be3 is strategic right now imho :)

Signed-off-by: Isabella do Amaral <[email protected]>
@isinyaaa isinyaaa force-pushed the push-nlxwsytwopkq branch 2 times, most recently from a8ee5e2 to aa2331a Compare October 28, 2024 18:20
@tarilabs
Copy link
Member

thanks a lot @isinyaaa , much appreciated !

@tarilabs tarilabs merged commit a1b2030 into containers:main Oct 30, 2024
12 checks passed
@isinyaaa isinyaaa deleted the push-nlxwsytwopkq branch October 30, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants