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.

Sign up for GitHub

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

Jump to bottom

feat: replace docker client code with oras - take 2 #1184

Merged
merged 20 commits into from
Dec 18, 2024
Merged

feat: replace docker client code with oras - take 2 #1184

merged 20 commits into from
Dec 18, 2024

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Dec 10, 2024 •
edited
Loading

What this PR does / why we need it

Closes open-component-model/ocm-project#302

Which issue(s) this PR fixes

@Skarlso Skarlso requested a review from a team as a code owner December 10, 2024 14:22
@github-actions github-actions bot added kind/feature new feature, enhancement, improvement, extension kind/dependency dependency update, etc. size/m Medium and removed kind/feature new feature, enhancement, improvement, extension labels Dec 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 10, 2024 •
edited
Loading

Mend Scan Summary: ❌

Repository: open-component-model/ocm

VIOLATION DESCRIPTION NUMBER OF VIOLATIONS
HIGH/CRITICAL SECURITY VULNERABILITIES 4
MAJOR UPDATES AVAILABLE 0
LICENSE REQUIRES REVIEW 2
HIGH RISK LICENSES 9
RESTRICTIED LICENSE FOR ON-PREMISE DELIVERY 0

Detailed Logs: mend-scan-> Generate Report
Mend UI

@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from f35726b to 82c5607 Compare December 10, 2024 14:26
@github-actions github-actions bot added the kind/feature new feature, enhancement, improvement, extension label Dec 10, 2024
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 24aacad to 7dd6512 Compare December 10, 2024 16:20
@github-actions github-actions bot added size/l Large and removed size/m Medium labels Dec 11, 2024
@github-actions github-actions bot added area/documentation Documentation related and removed area/documentation Documentation related labels Dec 12, 2024
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 7cadd7b to a3258ec Compare December 12, 2024 09:53
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from a3258ec to a51766c Compare December 12, 2024 10:34
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from a81eb4a to f7ce457 Compare December 16, 2024 11:14
@github-actions github-actions bot added size/m Medium and removed size/l Large labels Dec 16, 2024
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 1fc4a48 to 01b8c67 Compare December 16, 2024 12:51
@Skarlso
remove regctl and create the right oras client
78d2bc8 
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 01b8c67 to 78d2bc8 Compare December 16, 2024 13:00
@Skarlso Skarlso changed the title feat: replace docker client code with regclient feat: replace docker client code with oras - take 2 Dec 16, 2024
@Skarlso Skarlso added the area/ipcei Important Project of Common European Interest label Dec 16, 2024
jakobmoellerdev
jakobmoellerdev reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Mostly nits, generally LGTM but needs some clarification regarding concurrent access of readers

api/oci/extensions/repositories/ocireg/repository.go Outdated Show resolved Hide resolved
api/tech/docker/resolver.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Show resolved Hide resolved
api/tech/oras/delayed_reader.go Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
api/tech/oras/client.go Outdated Show resolved Hide resolved
@Skarlso
addressed feedback from PR review
31c23e8 
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

- addressed sync lock missing to ensure concurrent calls
- addressed code formatting issues
- addressed missing Logger transport augmentation
- addressed formatting and language issues
- removed unused docker code
@github-actions github-actions bot added size/l Large and removed size/m Medium labels Dec 17, 2024
@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 17, 2024

Something is not quite right, because I had to cancel the test. That means something is not okay with the added locking.

@jakobmoellerdev
Copy link
Contributor

If it fails in generate I believe this is a concurrency issue in the generator, do we have a global lock ack somewhere?

@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 17, 2024

@jakobmoellerdev Unsure because it didn't fail, it hang... which is worse. :D

@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 17, 2024

Okay, yeah, the mutex in the reader messes things up.

@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 73e1b47 to 934f789 Compare December 17, 2024 17:25
@Skarlso
Copy link
Contributor Author

Skarlso commented Dec 17, 2024

Yay all checks appear to be green.

@jakobmoellerdev
Copy link
Contributor

Why would the reader mutex cause a hangup though. That sounds off...

@Skarlso Skarlso force-pushed the replace-docker-with-regclient branch from 392e2aa to 43ed229 Compare December 18, 2024 08:46
@jakobmoellerdev jakobmoellerdev merged commit 104d276 into main Dec 18, 2024
23 checks passed
@jakobmoellerdev jakobmoellerdev deleted the replace-docker-with-regclient branch December 18, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianburth fabianburth fabianburth left review comments

@jakobmoellerdev jakobmoellerdev jakobmoellerdev approved these changes

Assignees
No one assigned
Labels
area/ipcei Important Project of Common European Interest kind/dependency dependency update, etc. kind/feature new feature, enhancement, improvement, extension size/l Large
Projects
OCM Backlog Board
Status: 🔒Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace the docker lib with oras
3 participants
@Skarlso @jakobmoellerdev @fabianburth