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

Add OCI providers + DockerHub and GHCR #2983

Merged
merged 2 commits into from
May 10, 2024
Merged

Add OCI providers + DockerHub and GHCR #2983

merged 2 commits into from
May 10, 2024

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Apr 7, 2024

Summary

This adds an initial implementation of OCI providers. This is very simple and is meant
to get this work kickstarted.

Currently it's safe to use this as it's not possible for folks to create custom providers.

The idea is to kickstart the methods and structures and elaborate on these as we go.

This cherry-picked the first two PRs from the POC in #2765

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@JAORMX JAORMX requested a review from a team as a code owner April 7, 2024 15:05
dmjb
dmjb reviewed Apr 8, 2024
View reviewed changes
cmd/dev/app/container/list.go Outdated Show resolved Hide resolved
@JAORMX JAORMX force-pushed the oci-providers branch 3 times, most recently from 56a6168 to 200b509 Compare April 11, 2024 12:45
@JAORMX JAORMX force-pushed the oci-providers branch 4 times, most recently from 4f2f20b to dc85935 Compare April 30, 2024 06:53
@coveralls
Copy link

coveralls commented Apr 30, 2024 •
edited
Loading

Coverage Status

coverage: 48.894%. first build
when pulling a47a3d0 on oci-providers
into e379a4a on main.

@JAORMX JAORMX force-pushed the oci-providers branch 4 times, most recently from 6833836 to 84e0706 Compare May 3, 2024 11:26
@JAORMX JAORMX changed the title Add scaffolding for OCI providers + DockerHub and GHCR Add OCI providers + DockerHub and GHCR May 3, 2024
@JAORMX JAORMX requested a review from dmjb May 3, 2024 12:56
@JAORMX JAORMX requested a review from jhrozek May 6, 2024 09:42
jhrozek
jhrozek reviewed May 6, 2024
View reviewed changes
cmd/dev/app/container/list.go Outdated

var prov provifv1.ImageLister
switch pclass.Value.String() {
case "dockerhub":
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use some constants here (e.g. PROVIDER_TYPE_DOCKERHUB from the proto file becuase that one is public)

jhrozek
jhrozek reviewed May 6, 2024
View reviewed changes
cmd/dev/app/container/list_tags.go Outdated

// print the containers
for _, container := range containers {
fmt.Println(container)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in the future we would use a table here for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that would be ideal.

@JAORMX JAORMX force-pushed the oci-providers branch 2 times, most recently from 17993bd to cca5ef1 Compare May 8, 2024 03:21
dmjb
dmjb reviewed May 8, 2024
View reviewed changes
proto/minder/v1/minder.proto Outdated
@@ -2096,6 +2106,9 @@ enum ProviderType {
PROVIDER_TYPE_GIT = 3 [(name) = "git"];
PROVIDER_TYPE_OCI = 4 [(name) = "oci"];
PROVIDER_TYPE_REPO_LISTER = 5 [(name) = "repo-lister"];
PROVIDER_TYPE_IMAGE_LISTER = 6 [(name) = "image-lister"];
PROVIDER_TYPE_GHCR = 7 [(name) = "ghcr"];
Copy link
Contributor

Choose a reason for hiding this comment

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

GHCR and Dockerhub are Provider classes, not types/traits, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, leftovers

@jhrozek
Copy link
Contributor

jhrozek commented May 8, 2024

The only thing that confuses me about this patchset is the classes and provider types, both in the DB and in the protobuf.

I thought that types map to interfaces, so we'd only have an OCI and repoLister type and github and ghcr would be classes, but in these patches we have github and ghcr as both.

I'll leave up to you if you want to get rid of the hardcoded strings before you merge.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@JAORMX
Implement OCI provider
b900bfc 
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@JAORMX
Add dockerhub and ghcr providers
3da2055 
Use a general container lister and implement sample command to test it
out

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 10, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@JAORMX JAORMX requested review from dmjb and jhrozek May 10, 2024 12:10
jhrozek
jhrozek approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

I think this is ready to be merged. I kicked the tests which are just flaky.

@JAORMX JAORMX merged commit 4c3fecd into main May 10, 2024
22 checks passed
@JAORMX JAORMX deleted the oci-providers branch May 10, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacklokbot stacklokbot stacklokbot left review comments

@jhrozek jhrozek jhrozek approved these changes

@dmjb dmjb Awaiting requested review from dmjb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JAORMX @coveralls @jhrozek @dmjb @stacklokbot