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

make func newDescriptor public #6517

Merged
merged 1 commit into from
Jan 12, 2024
Merged

make func newDescriptor public #6517

merged 1 commit into from
Jan 12, 2024

Conversation

antgubarev
Copy link

I compile bundle from key-value storage. But I can't implement DirectoryLoader for that beacause newDescriptor is private. I would like to use bundle.CustomReader. Is it possible?
Thanks!

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Hi @antgubarev 👋 And thanks for submitting this. Can you help me understand why you need newDescriptor to be public for your custom implementation of a directory loader? The Descriptor struct is public already, so could you not just use that directly?

@antgubarev
Copy link
Author

antgubarev commented Jan 10, 2024

Hi @antgubarev 👋 And thanks for submitting this. Can you help me understand why you need newDescriptor to be public for your custom implementation of a directory loader? The Descriptor struct is public already, so could you not just use that directly?

Hi @anderseknert !
I try to implement DirectoryLoader interface. Method NextFile returns Descriptor https://github.com/open-policy-agent/opa/blob/main/bundle/file.go#L123 . But I can't set descriptor's fields outside the bundle package. It's private https://github.com/open-policy-agent/opa/blob/main/bundle/file.go#L24

@anderseknert
Copy link
Member

Ah, right — I didn't think of checking the fields. I don't mind making the constructor public, not sure if anyone else has objections.

The DCO requirement needs to be fixed though. Just sign the commit like:

git commit --amend --signoff
git push --force

@ashutosh-narkar
Copy link
Member

Seems reasonable. We should probably make withCloser public too.

@srenatus
Copy link
Contributor

Seems reasonable. We should probably make withCloser public too.

Hmm we could also check if we've got an io.Reader that also an io.Closer and call its Close() opportunistically in (*Descriptor).Close(). Then we wouldn't need the withCloser anymore.

Copy link

netlify bot commented Jan 10, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 6823610
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/65a0298eca9358000861fd08
😎 Deploy Preview https://deploy-preview-6517--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.

@antgubarev
Copy link
Author

antgubarev commented Jan 10, 2024

@anderseknert I fixed my commit and made WithCloser public too

@ashutosh-narkar
Copy link
Member

@antgubarev can you please squash your commits and fix the sign-off ? We can then get this in.

@antgubarev
Copy link
Author

@ashutosh-narkar Sorry) Done.

@ashutosh-narkar
Copy link
Member

@antgubarev can you please fix the DCO again. Not sure what's happening.

Signed-off-by: Anton Gubarev <antgubarev.dev@gmail.com>
@antgubarev
Copy link
Author

@antgubarev can you please fix the DCO again. Not sure what's happening.

@ashutosh-narkar Fixed.

@ashutosh-narkar ashutosh-narkar merged commit 9c9b273 into open-policy-agent:main Jan 12, 2024
25 checks passed
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.

None yet

4 participants