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

Switch from crane package to remote #1244

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Switch from crane package to remote #1244

merged 1 commit into from
Sep 29, 2023

Conversation

errordeveloper
Copy link
Contributor

Signed-off-by: Ilya Dmitrichenko [email protected]

@errordeveloper errordeveloper force-pushed the switch-crane-v1-remote branch 2 times, most recently from 24d864b to 3f16ef0 Compare September 27, 2023 14:05
@stefanprodan stefanprodan marked this pull request as ready for review September 27, 2023 20:56
@stefanprodan stefanprodan marked this pull request as draft September 27, 2023 20:57
errordeveloper
errordeveloper commented Sep 28, 2023
View reviewed changes
internal/controller/ocirepository_controller_test.go
@@ -1867,7 +1878,7 @@ func TestOCIRepository_getArtifactURL(t *testing.T) {
{
name: "valid url with no reference",
url: "oci://ghcr.io/stefanprodan/charts",
want: "ghcr.io/stefanprodan/charts",
want: "ghcr.io/stefanprodan/charts:latest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanprodan @hiddeco the tests are passing locally, this is the only real change of a test case that I had to make that might be significant.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Sep 28, 2023 •
edited
Loading

@stefanprodan you mentioned retry logic is important. I cannot see it being configured in the code, so I wonder if there is something hidden in crane package that is not automatically provided by remote package that we will need to configure more explicitly.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Sep 28, 2023 •
edited
Loading

@stefanprodan you mentioned retry logic is important. I cannot see it being configured in the code, so I wonder if there is something hidden in crane package that is not automatically provided by remote package that we will need to configure more explicitly.

Looks like it is setup here: https://github.com/google/go-containerregistry/blob/dbcd01c402b2f05bcf6fb988014c5f37e9b13559/pkg/v1/remote/options.go#L166C2-L166C2
So I think it should all work as it did before.

@errordeveloper errordeveloper changed the title Switch from crane package to remote (WIP) Switch from crane package to remote Sep 28, 2023
@errordeveloper errordeveloper marked this pull request as ready for review September 28, 2023 20:56
@errordeveloper
Copy link
Contributor Author

I squashed WIP commits into one, it's ready for review now.

stefanprodan
stefanprodan approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Really nice refactoring! Thanks @errordeveloper 🥇

hiddeco
hiddeco approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thank you very much @errordeveloper. 💯

internal/controller/ocirepository_controller.go Outdated Show resolved Hide resolved
@errordeveloper
Switch from crane package to remote
a5ec631 
`crane` package is the highest level of abstraction that GGCR provides,
it's easy to use, however it doesn't give user much control.
This change moves `OCIRepository` controller logic to a lower-level
`remote` package and makes handling of references more explicit with
`name.Repository`, `name.Digest` and `name.Tag`.
It also simplifies options builder, as there is no need to have separate
sets of options for cosign and crane.

Signed-off-by: Ilya Dmitrichenko <[email protected]>
@stefanprodan stefanprodan merged commit 33dd859 into fluxcd:main Sep 29, 2023
10 checks passed
@hiddeco hiddeco added enhancement New feature or request area/oci OCI related issues and pull requests labels Sep 29, 2023
This was referenced Sep 29, 2023
Open
Open
@errordeveloper errordeveloper mentioned this pull request Feb 8, 2024
Draft
@souleb souleb mentioned this pull request Mar 4, 2024
Open
3 tasks
@maboehm maboehm mentioned this pull request May 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@hiddeco hiddeco hiddeco approved these changes

Assignees
No one assigned
Labels
area/oci OCI related issues and pull requests enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@errordeveloper @stefanprodan @hiddeco