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

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

Merged
merged 1 commit into from Sep 6, 2022

Conversation

JoshVanL
Copy link
Contributor

Fixes #29

`core/v1` -> `v1`.

Signed-off-by: joshvanl <me@joshvanl.dev>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 23, 2022
@JoshVanL
Copy link
Contributor Author

/assign @munnerz

@jetstack-bot jetstack-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2022
@jetstack-bot
Copy link
Contributor

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@munnerz
Copy link
Member

munnerz commented Sep 1, 2022

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
Copy link
Contributor Author

JoshVanL commented Sep 6, 2022

@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.

@munnerz
Copy link
Member

munnerz commented Sep 6, 2022

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2022
@jetstack-bot jetstack-bot merged commit 213ec03 into cert-manager:main Sep 6, 2022
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Sep 6, 2022

@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
Copy link
Member

munnerz commented Sep 6, 2022

@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
Copy link
Member

munnerz commented Sep 6, 2022

@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 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CertificateRequests created by the csi driver are not garbage collected due to incorrect apiVersion
4 participants