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

feat(download): add opa_no_oci flag to build without containerd #6159

Merged

Conversation

slonka
Copy link
Contributor

@slonka slonka commented Aug 17, 2023

xrel #6160

Why the changes in this PR are needed?

I think it's best explained in the issue - #6160

What are the changes in this PR?

This pr adds an opa_no_oci build flag to exclude containerd dependency.

Notes to assist PR review:

Look at the issue first - #6160.

Further comments:

Nothing.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Skimmed it. Let's collect more input and see if an ideal solution -- removing containerd while keeping the functionality -- would be another option... 😃

download/oci_download.go Outdated Show resolved Hide resolved
download/oci_download_unavailable.go Outdated Show resolved Hide resolved
@slonka slonka marked this pull request as ready for review August 17, 2023 12:16
@slonka slonka changed the title feat(download): add no_oci flag to build without containerd feat(download): add opa_no_oci flag to build without containerd Aug 17, 2023
@ashutosh-narkar
Copy link
Member

As Stephan mentioned, removing the containerd dependency would be ideal. A cursory look at the OCI code makes me think we should be able to do that. @DerGut wdyt? That of course would be more involved but could be better from a maintenance pov.

The approach in this PR seems fine. I guess we could have additional such flags in the future to control functionality which does not sound ideal.

@slonka
Copy link
Contributor Author

slonka commented Aug 23, 2023

Hey @DerGut 👋 - would love your input on this by the end of this week. We have a release planed next week so would love to know if this could be merged by then or not.

@DerGut
Copy link
Contributor

DerGut commented Aug 23, 2023

Hi @slonka and @ashutosh-narkar I'm having a look now. I try to give a response by tomorrow 👍

@DerGut
Copy link
Contributor

DerGut commented Aug 24, 2023

It definitely seems possible to remove containerd as a dependency and only use functionality provided by oras.

We can use the remote.Repository type to pass it to the oras.Copy call.
Oras also provides some interfaces for authentication, there is the auth.Client that can be set on the repository.

I've played around with it a little bit but couldn't yet get all tests to pass. To integrate our authentication plugins with oras's auth interface, more in-depth understanding would be needed.

@slonka if you need to cut a release by the end of the week, I'd suggest to merge this PR first and iterate on removing containerd with a bit more time. I could see myself taking this on. @ashutosh-narkar what do you think?

@ashutosh-narkar
Copy link
Member

Removing the containerd dep would be ideal and @DerGut if you have bandwidth to look into it that would be great! For now this change seems fine. @srenatus any other thoughts here?

@srenatus
Copy link
Contributor

Let's call it out as temporary in the release notes (or omit it), though -- if the containerd dep was gone, there's no need anymore to disable the functionality; so we could revert this in the future.

@ashutosh-narkar
Copy link
Member

@slonka can you please squash your commits and we can then get this in. Thanks!

@netlify
Copy link

netlify bot commented Aug 24, 2023

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit b83341b
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/64e7a423dbcad500072ef055
😎 Deploy Preview https://deploy-preview-6159--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: slonka <slonka@users.noreply.github.com>
@slonka
Copy link
Contributor Author

slonka commented Aug 24, 2023

@ashutosh-narkar the tests seem to be flaky (different ones fail on the same changes) - can you re-run them?

@ashutosh-narkar
Copy link
Member

@ashutosh-narkar the tests seem to be flaky (different ones fail on the same changes) - can you re-run them?

The test failures are not related to your changes. We're looking into it.

@ashutosh-narkar ashutosh-narkar merged commit 105946a into open-policy-agent:main Aug 24, 2023
22 of 24 checks passed
@slonka
Copy link
Contributor Author

slonka commented Aug 25, 2023

@ashutosh-narkar - last question: is it safe for us to use main branch or should we wait for this to be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants