kapp
kapp copied to clipboard
carvel-dev
Include default template rules for packaging resources
What this PR does / why we need it:
This PR include default templateRules for kapp-controller's packaging resources; PackageRepository, PackageInstall and App. Since kapp and kapp-controller are companions it seems helpful.
In particular, the following templateRules are added to the defaults:
-
PackageRepository(according to spec).spec.fetch.inline.pathsFrom[].secretRef.spec.fetch.inline.pathsFrom[].configMapRef
-
PackageInstall(according to spec).spec.values[].secretRef.spec.cluster.kubeconfigSecretRef
-
App(according to spec).spec.cluster.kubeconfigSecretRef.spec.fetch[].inline.pathsFrom[].secretRef.spec.fetch[].inline.pathsFrom[].configMapRef.spec.fetch[].imgpkgBundle.secretRef.spec.fetch[].http.secretRef.spec.fetch[].git.secretRef.spec.fetch[].helmChart.repository.secretRef.spec.template[].ytt.inline.pathsFrom[].secretRef.spec.template[].ytt.inline.pathsFrom[].configMapRef.spec.template[].ytt.valuesFrom[].secretRef.spec.template[].ytt.valuesFrom[].configMapRef.spec.template[].helmTemplate.valuesFrom[].secretRef.spec.template[].helmTemplate.valuesFrom[].configMapRef.spec.template[].cue.valuesFrom[].secretRef.spec.template[].sops.pgp.privateKeySecretRef
Which issue(s) this PR fixes:
Fixes #596
Does this PR introduce a user-facing change?
Include default template rules for _kapp-controller_'s `PackageRepository`, `PackageInstall` and `App` resources.
Questions for the maintainers:
-
Is this change wanted at all? From what I can tell, additional template rules are a non-invasive / non-breaking change. However, I can't intuit if this could have unwanted effects on kapp-controller.
-
After studying the code base, I think to understand that there are no (focused) tests for the default config in general and its template rules in particular. However, this change needs to be tested. Before I invest, I would love to know the maintainers recommendation for covering this. There are unit tests for versioned resources and e2e tests. I imagine tests for the default config to sit in between them, e.g. maybe a
default_test.gowhich containsTestDefaultTemplateRules_{PackageRepository, PackageInstall, App}.In the meantime, I have manually tested the new rules. See details in comment.
Additional Notes for your reviewer:
Review Checklist:
- [ ] Follows the developer guidelines
- [ ] Relevant tests are added or updated
- [ ] Relevant docs in this repo added or updated
- [ ] Relevant carvel.dev docs added or updated in a separate PR and there's a link to that PR
- [ ] Code is at least as readable and maintainable as it was before this change
Additional documentation e.g., Proposal, usage docs, etc.:
(see PR description for Q about testing)
In the meantime, here's how I've manually tested this change so far.
#! test.yaml
---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageRepository
metadata:
name: test
spec:
fetch:
inline:
pathsFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
name: test
spec:
values:
- secretRef:
name: test-secret
cluster:
kubeconfigSecretRef:
name: test-secret
---
apiVersion: kappctrl.k14s.io/v1alpha1
kind: App
metadata:
name: test
spec:
cluster:
kubeconfigSecretRef:
name: test-secret
fetch:
- inline:
pathsFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
- imgpkgBundle:
secretRef:
name: test-secret
- http:
secretRef:
name: test-secret
- git:
secretRef:
name: test-secret
- helmChart:
repository:
secretRef:
name: test-secret
template:
- ytt:
inline:
pathsFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
valuesFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
- helmTemplate:
valuesFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
- cue:
valuesFrom:
- secretRef:
name: test-secret
- configMapRef:
name: test-configmap
- sops:
pgp:
privateKeySecretRef:
name: test-secret
---
apiVersion: v1
kind: Secret
metadata:
name: test-secret
annotations:
kapp.k14s.io/versioned: ""
data:
config.yml: ""
---
apiVersion: v1
kind: ConfigMap
metadata:
name: test-configmap
annotations:
kapp.k14s.io/versioned: ""
data:
config.yml: ""
go run cmd/kapp/kapp.go deploy --app test --yes --diff-changes --diff-run --file test.yaml
Target cluster 'https://127.0.0.1:56481' (nodes: kind-control-plane)
@@ create secret/test-secret-ver-1 (v1) namespace: default @@
0 + apiVersion: v1
1 + data:
2 + config.yml: <-- value not shown (#1)
3 + kind: Secret
4 + metadata:
5 + annotations:
6 + kapp.k14s.io/versioned: ""
7 + labels:
8 + kapp.k14s.io/app: "1663512989648583000"
9 + kapp.k14s.io/association: v1.2af780e8ffab3a405b9c89686418b58a
10 + name: test-secret-ver-1
11 + namespace: default
12 +
@@ create configmap/test-configmap-ver-1 (v1) namespace: default @@
0 + apiVersion: v1
1 + data:
2 + config.yml: ""
3 + kind: ConfigMap
4 + metadata:
5 + annotations:
6 + kapp.k14s.io/versioned: ""
7 + labels:
8 + kapp.k14s.io/app: "1663512989648583000"
9 + kapp.k14s.io/association: v1.3b489d168caf3f033eb1d98e482fc052
10 + name: test-configmap-ver-1
11 + namespace: default
12 +
@@ create packagerepository/test (packaging.carvel.dev/v1alpha1) namespace: default @@
0 + apiVersion: packaging.carvel.dev/v1alpha1
1 + kind: PackageRepository
2 + metadata:
3 + labels:
4 + kapp.k14s.io/app: "1663512989648583000"
5 + kapp.k14s.io/association: v1.1d9d3c5a5fa754c7715d4707b9774382
6 + name: test
7 + namespace: default
8 + spec:
9 + fetch:
10 + inline:
11 + pathsFrom:
12 + - secretRef:
13 + name: test-secret-ver-1
14 + - configMapRef:
15 + name: test-configmap-ver-1
16 +
@@ create packageinstall/test (packaging.carvel.dev/v1alpha1) namespace: default @@
0 + apiVersion: packaging.carvel.dev/v1alpha1
1 + kind: PackageInstall
2 + metadata:
3 + labels:
4 + kapp.k14s.io/app: "1663512989648583000"
5 + kapp.k14s.io/association: v1.8da39a73cfc4136f8ac556df26e520ea
6 + name: test
7 + namespace: default
8 + spec:
9 + cluster:
10 + kubeconfigSecretRef:
11 + name: test-secret-ver-1
12 + values:
13 + - secretRef:
14 + name: test-secret-ver-1
15 +
@@ create app/test (kappctrl.k14s.io/v1alpha1) namespace: default @@
0 + apiVersion: kappctrl.k14s.io/v1alpha1
1 + kind: App
2 + metadata:
3 + labels:
4 + kapp.k14s.io/app: "1663512989648583000"
5 + kapp.k14s.io/association: v1.93248f12c712bc5436fdd56b8be5641d
6 + name: test
7 + namespace: default
8 + spec:
9 + cluster:
10 + kubeconfigSecretRef:
11 + name: test-secret-ver-1
12 + fetch:
13 + - inline:
14 + pathsFrom:
15 + - secretRef:
16 + name: test-secret-ver-1
17 + - configMapRef:
18 + name: test-configmap-ver-1
19 + - imgpkgBundle:
20 + secretRef:
21 + name: test-secret-ver-1
22 + - http:
23 + secretRef:
24 + name: test-secret-ver-1
25 + - git:
26 + secretRef:
27 + name: test-secret-ver-1
28 + - helmChart:
29 + repository:
30 + secretRef:
31 + name: test-secret-ver-1
32 + template:
33 + - ytt:
34 + inline:
35 + pathsFrom:
36 + - secretRef:
37 + name: test-secret-ver-1
38 + - configMapRef:
39 + name: test-configmap-ver-1
40 + valuesFrom:
41 + - secretRef:
42 + name: test-secret-ver-1
43 + - configMapRef:
44 + name: test-configmap-ver-1
45 + - helmTemplate:
46 + valuesFrom:
47 + - secretRef:
48 + name: test-secret-ver-1
49 + - configMapRef:
50 + name: test-configmap-ver-1
51 + - cue:
52 + valuesFrom:
53 + - secretRef:
54 + name: test-secret-ver-1
55 + - configMapRef:
56 + name: test-configmap-ver-1
57 + - sops:
58 + pgp:
59 + privateKeySecretRef:
60 + name: test-secret-ver-1
61 +
Changes
Namespace Name Kind Age Op Op st. Wait to Rs Ri
default test App - create - reconcile - -
^ test PackageInstall - create - reconcile - -
^ test PackageRepository - create - reconcile - -
^ test-configmap-ver-1 ConfigMap - create - reconcile - -
^ test-secret-ver-1 Secret - create - reconcile - -
Op: 5 create, 0 delete, 0 update, 0 noop, 0 exists
Wait to: 5 reconcile, 0 delete, 0 noop
Succeeded
Hey @mamachanko ! So sorry I totally responded to this in my head. I verified that all Secret and ConfigMap references. Would you be open to adding a test for this? I will look up and drop links to similar tests if that helps!
I verified that all Secret and ConfigMap references.
Nice. Thanks for going over them!
Would you be open to adding a test for this?
Yes, I'd be happy to. I will test a few limited cases first and check in with you to vet the approach before going through the full lot.
@100mik I won't be able to add tests right away, because time and obligations. There's no rush here, is that right?
In the meantime, if you are able to point me at existing tests, that'd be helpful. However, as far as I can tell, no tests exercise the default configuration's templating rules yet.
Hey! Absolutely no rush from our side.
Today, the only test we have for template rules is an E2E test. I think the example you shared above is perfect ! It can be captured in an E2E test in a similar manner (like the existing one).
It might be cool to figure out if we can have tabular unit tests for template rules. But that would definitely be a separate PR altogether 🤔
@100mik
I think the example you shared above is perfect ! It can be captured in an E2E test in a similar manner (like the existing one)
To have an e2e test with example shared by @mamachanko will require installation and deletion of kapp-controller as well, as CRDs of PackageInstall, App and PackageRepository will require to deploy these resources. I think installation of kapp-controller will be bit heavy in e2e test. Alternative of this could be adding of CRDs of these resources in the manifest instead of using kapp-controller might be helpful.
Yep that makes sense, I think deploying the latest kapp-controller release while applying only CRDs using filters.
And we just need to check the diff, so we can set --wait=false while testing the template rules 🤔
@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.
Currently, it only contains a single entry in its test table for demonstration. However, it can be easily extended with all remaining packaging-related - or in fact any - template rule test.
Before I invest and fill in the missing cases, would you be able to have a look and see if this makes sense to you? Do you like the level of test? Are the APIs and types used idiomatically? Is there are simpler way?
Ty! 🙇🏻
@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.
@mamachanko I was also thinking to have unit test instead of E2E for this scenario. For template rules we already have E2E test via versioned resources. I am fine with this. 👍
@kumaritanushree @100mik Before committing to an E2E test scenario, I wanted to explore a more unit-style approach. After discovering all the required types and APIs I drafted a table-driven default_template_rules_test.go.
@mamachanko I was also thinking to have unit test instead of E2E for this scenario. For template rules we already have E2E test via versioned resources. I am fine with this. 👍
Thank you!
default_template_rules_test.go now tests the newly added template rules for kapp-controller's resources.
The PR is ready for review. 🔍
default_template_rules_test.gonow tests the newly added template rules for kapp-controller's resources.
@mamachanko I think file name should be default_test.go so that if in future if required we can add test for other rules as well in the same file.
default_template_rules_test.go sounds like this will have only test for template rules.
default_template_rules_test.gonow tests the newly added template rules for kapp-controller's resources.@mamachanko I think file name should be default_test.go so that if in future if required we can add test for other rules as well in the same file.
default_template_rules_test.gosounds like this will have only test for template rules.
@kumaritanushree Great point. Renamed the file. ✔️