Skip to content

Helm tweaks + better makefile "ensure"#97

Merged
jetstack-bot merged 7 commits intocert-manager:mainfrom
SgtCoDFish:helmake
Jan 19, 2023
Merged

Helm tweaks + better makefile "ensure"#97
jetstack-bot merged 7 commits intocert-manager:mainfrom
SgtCoDFish:helmake

Conversation

@SgtCoDFish
Copy link
Member

@SgtCoDFish SgtCoDFish commented Jan 16, 2023

See individual commits for details

@SgtCoDFish SgtCoDFish added this to the v0.4.0 milestone Jan 16, 2023
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2023
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jan 16, 2023
@SgtCoDFish
require k8s 1.22 in trust-manager, improve description in chart
57f6cdf 
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@SgtCoDFish
add a NOTES file with instructions about updating trust packages
c5b4602 
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@SgtCoDFish
default to using only kubeconfig for trust demo cluster
a35ea43 
also ensure that permissions are set correctly and use the 'latest' tag
for the debian trust package when building a demo cluster

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
irbekrm
irbekrm reviewed Jan 17, 2023
View reviewed changes
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks @SgtCoDFish I've added a couple nits and comments.

Generally from a perspective of someone who is going to perhaps deploy this project together with some other projects I prefer an approach where make targets for creating a (kind) cluster and deploying the actual software don't depend on each other, so if I already have a cluster with some stuff and I want to deploy trust-manager to that particular cluster, I have a straightforward make target where I can pass a kubeconfig or kind cluster name and it will do it.
However, that approach is also not perfect as if you want to load images it'll only work for kind clusters anyway. So happy to approve minus a couple nits and comments 👍🏼

@SgtCoDFish
Copy link
Member Author

SgtCoDFish commented Jan 17, 2023

Generally from a perspective of someone who is going to perhaps deploy this project together with some other projects I prefer an approach where make targets for creating a (kind) cluster and deploying the actual software don't depend on each other, so if I already have a cluster with some stuff and I want to deploy trust-manager to that particular cluster, I have a straightforward make target where I can pass a kubeconfig or kind cluster name and it will do it.

I think that's a totally valid thing but I'm not sure from the eyes of an end-user it's how I'd see it. As an end-user I ideally wouldn't be cloning the repo to use the makefile - I'd be installing from Helm, because that's the supported way.

Most of this is very dev-focused, I guess?

@irbekrm
Copy link
Contributor

irbekrm commented Jan 17, 2023

I think that's a totally valid thing but I'm not sure from the eyes of an end-user it's how I'd see it. As an end-user I ideally wouldn't be cloning the repo to use the makefile - I'd be installing from Helm, because that's the supported way.
Most of this is very dev-focused, I guess?

Yeah I also meant it from dev perspective - I could have scripts that I use that create a certain cluster configuration with a certain name where I want to install trust or I might want to install two cert-manager related projects from latest master to the same cluster- in both cases it'd be easier if they don't require a specific cluster name.
But this is just a non-actionable note- I don't want to block this PR which is overall valuable with this.

@SgtCoDFish
add more make help descriptions
6e1fd51 
also add CI var default, use kubeconfig flags everywhere

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@SgtCoDFish
subtle: change how flags are passed to smoke tests
b4ecb96 
This never worked previously because it was set up wrong.

This is documented on the ginkgo site but it's not obvious:

https://onsi.github.io/ginkgo/\#supporting-custom-configuration-custom-command-line-flags

This also moves the init to the "smoke_test.go" file rather than having
it in "smoke.go". This was required for the flags to be respected, and
in any case "smoke.go" just seems confusing when this directory /
package is entirely about testing.

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@SgtCoDFish
do cluster setup in kubeconfig target
3f3559a 
this allows us to write the kubeconfig at cluster creation, which is
slightly cleaner than having to write it just after creation.

this also prevents kind from changing the default kubeconfig

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 18, 2023
@SgtCoDFish
Copy link
Member Author

SgtCoDFish commented Jan 18, 2023

I've pushed the requested changes and a couple more:

  • Fixed smoke test argument passing so kubeconfig-path is actually used. It never previously worked!
  • Maybe more controversial: Only writes kubeconfig into bin, so that $HOME/.kube/config isn't touched. I'm kinda unsure about that, since it'll make it a little harder to play with a local demo cluster, but I like that it exposes bugs like the smoke test thing above. WDYT?

@irbekrm
Copy link
Contributor

irbekrm commented Jan 18, 2023

Thanks for making the changes @SgtCoDFish !

Maybe more controversial: Only writes kubeconfig into bin, so that $HOME/.kube/config isn't touched. I'm kinda unsure about that, since it'll make it a little harder to play with a local demo cluster, but I like that it exposes bugs like the smoke test thing above. WDYT?

I think it makes sense to do whatever is needed to make the tests less flakey.
I do think that it makes it harder to play with a local demo cluster- to me this seems to be another argument to split the local deploy flow into separate make targets that aren't dependent on each other, so that a user can run a combination of
make ensure-kind, make load-images KIND_CLUSTER_NAME=X, KUBECONFIG=./X, make deploy-trust-manager KUBECONFIG=./X. Or they can run a single target that does all of that with less configurability that the e2e tests also use if they are okay with it being a bit harder to make the cluster usable for other scenarios. However, that'd probably not be in this PR.

In this PR, perhaps we could print something like kind cluster X was created, if you want to access it run kind export kubeconfig --name trust in the target that creates the cluster?
Otherwise happy to approve this PR.

@SgtCoDFish
Copy link
Member Author

SgtCoDFish commented Jan 19, 2023

In this PR, perhaps we could print something like kind cluster X was created, if you want to access it run kind export kubeconfig --name trust in the target that creates the cluster?

I like this, makes a lot of sense. Will add 👍

@SgtCoDFish
add informational output when creating a kind cluster
a7c6440 
also adds some basic colors in output when not in CI

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
irbekrm
irbekrm approved these changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and for making all the changes @SgtCoDFish !

This looks good to go in to me 👍🏼 I think it's definitely an improvement

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2023
@jetstack-bot
Copy link
Contributor

jetstack-bot commented Jan 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, SgtCoDFish

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

The pull request process is described here

Details 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

@jetstack-bot jetstack-bot merged commit 54443a2 into cert-manager:main Jan 19, 2023
@SgtCoDFish SgtCoDFish deleted the helmake branch January 19, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@irbekrm irbekrm irbekrm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@irbekrm irbekrm

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v0.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@SgtCoDFish @irbekrm @jetstack-bot