Pass additional information to downward API (k8s version, kc version, k8s g/v) #846

neil-hickey · 2022-08-25T18:20:38Z

Signed-off-by: Neil Hickey [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #781

Does this PR introduce a user-facing change?

Templating stages (ytt, helm, cue) now have the kubernetes version, available group/version APIs and kapp-controller version passed to them via data values, and enabled capabilites in helm.

Additional Notes for your reviewer:

Review Checklist:

Follows the developer guidelines
Relevant tests are added or updated
Code is at least as readable and maintainable as it was before this
change
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

Additional documentation e.g., Proposal, usage docs, etc.:

pkg/app/app_reconcile_test.go

pkg/app/app.go

Signed-off-by: Neil Hickey <[email protected]>

joe-kimmel-vmw

I have some comments and questions but i wouldn't argue with merging this as-is. we can catch up real quick tomorrow AM about whether it's worth addressing any of these in this iteration.

cmd/controller/run.go

pkg/componentInfo/component_info.go

pkg/deploy/factory.go

pkg/componentInfo/component_info.go

pkg/packageinstall/packageinstall.go

pkg/template/downward_api_values.go

pkg/template/values_factory.go

joaopapereira

Added some comments

cmd/controller/run.go

pkg/componentInfo/component_info.go

pkg/deploy/kapp.go

joaopapereira · 2022-08-29T21:15:54Z

pkg/componentInfo/component_info.go

+}
+
+// NewComponentInfo returns a ComponentInfo
+func NewComponentInfo(coreClient kubernetes.Interface, clusterAccess *kubeconfig.Kubeconfig, kappControllerVersion string) *ComponentInfo {


The clusterAccess parameter could also be an interface.

pkg/template/helm_template.go

joaopapereira · 2022-08-30T18:16:28Z

pkg/template/values.go

-	coreClient kubernetes.Interface
+	appContext                  AppContext
+	coreClient                  kubernetes.Interface
+	additionalDownwardAPIValues AdditionalDownwardAPIValues
 }

 func (t Values) AsPaths(dirPath string) ([]string, func(), error) {


Just a comment about this function and maybe not be actionable in this PR.
This signature feels strange, the main reason for that is the return of that func() that you need to read the code to understand that it is the removal of the created folders.
It is the responsibility of the creator of dirPath to delete that folder, while this function is only responsible for creating the folders it needs inside the dirPath.

Not going to action on this one in this PR

- renaming some things - moved away from a values factory back to values struct - minor fixups Signed-off-by: Neil Hickey <[email protected]>

pkg/app/app_template.go

cppforlife · 2022-09-01T13:28:06Z

pkg/packageinstall/packageinstall.go

+	v.Pre = semver.PRVersion{}
+	v.Build = semver.BuildMeta{}


should this move into ComponentInfo? seems its more related to version parsing rather than package matching

The constraint matching is specific to packageinstall (i.e the choice to drop the pre and build fields of KC version), so I think it makes more sense here. The kapp-controller version should always be as it is in the cluster, regardless of how / why we manipulate it in kapp-controller.

I understand the point that it's not entirely semantically in-scope for the reader of PackageInstall to know about the effects of resetting the members of the version, v struct. However it's less obvious to me where to move that logic, and i think it'll be a similar number of lines of code, just maybe a little faster to read packageinstall.go until your concern is this logic specifically. To the extent the concern is just readability, it feels like a refactor or an explanatory comment would have similar benefits.

pkg/apis/kappctrl/v1alpha1/types_template.go

Signed-off-by: Neil Hickey <[email protected]>

pkg/packageinstall/packageinstall.go

- Add template() test to validate memoizing works Signed-off-by: Neil Hickey <[email protected]>

- memoizing within packageinstall didnt actually memoize - memoizing within componentinfo was too aggressive so controller would not receive updated version after cluster is updated

vmwclabot added the cla-not-required label Aug 25, 2022

neil-hickey commented Aug 25, 2022

View reviewed changes

pkg/app/app_reconcile_test.go Show resolved Hide resolved

neil-hickey force-pushed the nh-versions-downward-api-expansion branch 2 times, most recently from 0483769 to e49b956 Compare August 25, 2022 19:07

neil-hickey commented Aug 25, 2022

View reviewed changes

pkg/app/app.go Outdated Show resolved Hide resolved

neil-hickey force-pushed the nh-versions-downward-api-expansion branch 6 times, most recently from e5dd634 to 48c5c5c Compare August 25, 2022 22:43

neil-hickey changed the title ~~WIP: Retrieve version information prior to fet,temp,deploy steps~~ Pass additional information to downward API (k8s version, kc version, k8s g/v) Aug 25, 2022

WIP: Retrieve version information prior to fet,temp,deploy steps

f3a8c23

Signed-off-by: Neil Hickey <[email protected]>

neil-hickey force-pushed the nh-versions-downward-api-expansion branch 2 times, most recently from 9180374 to d91178f Compare August 26, 2022 20:13

Fix linter / import statements

084e920

Signed-off-by: Neil Hickey <[email protected]>

neil-hickey force-pushed the nh-versions-downward-api-expansion branch from d91178f to 084e920 Compare August 26, 2022 21:00

neil-hickey marked this pull request as ready for review August 29, 2022 16:08

neil-hickey mentioned this pull request Aug 29, 2022

Versions Spike Exploration PR #833

Closed

joe-kimmel-vmw approved these changes Aug 29, 2022

View reviewed changes

joaopapereira reviewed Aug 30, 2022

View reviewed changes

neil-hickey force-pushed the nh-versions-downward-api-expansion branch 2 times, most recently from 4714056 to 69e9c3b Compare August 30, 2022 23:52

Address review comments

e2dc9c7

- renaming some things - moved away from a values factory back to values struct - minor fixups Signed-off-by: Neil Hickey <[email protected]>

neil-hickey force-pushed the nh-versions-downward-api-expansion branch from 69e9c3b to e2dc9c7 Compare August 31, 2022 20:50

cppforlife reviewed Sep 1, 2022

View reviewed changes

pkg/app/app_template.go Outdated Show resolved Hide resolved

cppforlife reviewed Sep 1, 2022

View reviewed changes

pkg/app/app_template.go Outdated Show resolved Hide resolved

cppforlife reviewed Sep 1, 2022

View reviewed changes

pkg/apis/kappctrl/v1alpha1/types_template.go Outdated Show resolved Hide resolved

cppforlife reviewed Sep 1, 2022

View reviewed changes

pkg/apis/kappctrl/v1alpha1/types_template.go Show resolved Hide resolved

Fixup errors in app_template

efcd038

Signed-off-by: Neil Hickey <[email protected]>

neil-hickey force-pushed the nh-versions-downward-api-expansion branch 3 times, most recently from ba3e515 to 46cacc6 Compare September 2, 2022 17:26

joe-kimmel-vmw reviewed Sep 2, 2022

View reviewed changes

pkg/packageinstall/packageinstall.go Outdated Show resolved Hide resolved

Add memoized fetching of versions

7f0abc2

- Add template() test to validate memoizing works Signed-off-by: Neil Hickey <[email protected]>

neil-hickey force-pushed the nh-versions-downward-api-expansion branch from 46cacc6 to 7f0abc2 Compare September 2, 2022 19:04

Dmitriy Kalinin added 2 commits September 6, 2022 18:50

remove unnecessary memoization

38db77e

- memoizing within packageinstall didnt actually memoize - memoizing within componentinfo was too aggressive so controller would not receive updated version after cluster is updated

rename kubernetesGroupVersions to kubernetesAPIs

3d9f87f

cppforlife force-pushed the nh-versions-downward-api-expansion branch from 2e50459 to 43408dc Compare September 7, 2022 14:23

use array as type of values for kubernetesAPIs

5969595

cppforlife force-pushed the nh-versions-downward-api-expansion branch from 43408dc to 5969595 Compare September 7, 2022 17:23

cppforlife merged commit 870f5a1 into develop Sep 7, 2022

cppforlife deleted the nh-versions-downward-api-expansion branch September 7, 2022 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass additional information to downward API (k8s version, kc version, k8s g/v) #846

Pass additional information to downward API (k8s version, kc version, k8s g/v) #846

neil-hickey commented Aug 25, 2022 •

edited

Loading

joe-kimmel-vmw left a comment

joaopapereira left a comment

joaopapereira Aug 29, 2022

joaopapereira Aug 30, 2022

neil-hickey Sep 2, 2022

cppforlife Sep 1, 2022

neil-hickey Sep 1, 2022 •

edited

Loading

joe-kimmel-vmw Sep 2, 2022 •

edited

Loading

Pass additional information to downward API (k8s version, kc version, k8s g/v) #846

Pass additional information to downward API (k8s version, kc version, k8s g/v) #846

Conversation

neil-hickey commented Aug 25, 2022 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional Notes for your reviewer:

Review Checklist:

Additional documentation e.g., Proposal, usage docs, etc.:

joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

joaopapereira left a comment

Choose a reason for hiding this comment

joaopapereira Aug 29, 2022

Choose a reason for hiding this comment

joaopapereira Aug 30, 2022

Choose a reason for hiding this comment

neil-hickey Sep 2, 2022

Choose a reason for hiding this comment

cppforlife Sep 1, 2022

Choose a reason for hiding this comment

neil-hickey Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

joe-kimmel-vmw Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

neil-hickey commented Aug 25, 2022 •

edited

Loading

neil-hickey Sep 1, 2022 •

edited

Loading

joe-kimmel-vmw Sep 2, 2022 •

edited

Loading