search

cert-manager / csi-lib

A library for building CSI drivers that request certificates from cert-manager
Apache License 2.0
14 stars 13 forks source link

Change CertificateRequest owner reference of pod `core/v1` -> `v1` #31

Closed JoshVanL closed 2 years ago

JoshVanL commented 2 years ago

Fixes #29

JoshVanL commented 2 years ago

/assign @munnerz

jetstack-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 7ing, JoshVanL

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/cert-manager/csi-lib/blob/main/OWNERS)~~ [JoshVanL] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
munnerz commented 2 years ago

Can we add an e2e test that:

1) creates a pod that uses a CSI volume 2) asserts a CR exists 3) deletes the pod 4) asserts the CR does not exist

Otherwise, lgtm :)

JoshVanL commented 2 years ago

@munnerz yes, sounds good to me!

The project doesn't have any e2e dep setup yet, so will do that in a separate PR containing the boilerplate. How do we feel about experimenting with nix for managing the deps? :slightly_smiling_face:

I am of the opinion that nix flakes is growing in popularity and will become a "default" dependency management and distribution tool. csi-lib is a good project to experiment with given it has minimal end-user facing interaction and there is not much (basically only the current Dockerfile) to convert.

munnerz commented 2 years ago

/lgtm

JoshVanL commented 2 years ago

@munnerz sorry, wasn’t clear and should have added a hold 😅 I meant let’s hold this PR and rebase on top of the e2e test boilerplate once that had merged. No biggie, I’ll create an issue.

munnerz commented 2 years ago

@munnerz yes, sounds good to me!

The project doesn't have any e2e dep setup yet, so will do that in a separate PR containing the boilerplate. How do we feel about experimenting with nix for managing the deps? 🙂

I am of the opinion that nix flakes is growing in popularity and will become a "default" dependency management and distribution tool. csi-lib is a good project to experiment with given it has minimal end-user facing interaction and there is not much (basically only the current Dockerfile) to convert.

I'm impartial, though given the extra complexity & knowledge required to manage/operate a nix based setup (of which as a project I don't think we have much experience), I'm wary to introduce a whole new ecosystem (especially as, in this case, we already have prior art for setting up dependencies).

This is a good place to experiment given the limited developer community around it, and minimal size. That said, for me at least there are a lot of unknown unknowns, which makes me wary to give a firm "let's do it 😄".

munnerz commented 2 years ago

@munnerz sorry, wasn’t clear and should have added a hold 😅 I meant let’s hold this PR and rebase on top of the e2e test boilerplate once that had merged. No biggie, I’ll create an issue.

😄 woops! But I think this is a valuable change for us to get merged anyway, especially as this is in the csi-lib (and we can leave it untagged if we aren't yet confident in its results) and is a really narrow change. An issue would be great 🥳