feat: parameterization by ericzzzzzzz · Pull Request #8365 · GoogleContainerTools/skaffold

ericzzzzzzz · 2023-01-25T19:03:49Z

Fixes: #8245, #7954

Future work: #8306, #8406, #8410 add kpt examples

Description:

move transformers responsibility out of specific kpt render as
- transformers and validators are KRM functions, those functions should not be tied to a specific KRM function executors(kpt) in this case. We may support running krm functions without kpt(kustomize, or internal implementation).
- Kustomize removes comments when building manifests, the previous approach for using transformers doesn't work for manifests with comments.
- creating extra hidden directory is not necessary, also it's contaminating users' workspace
- kpt render command outputs Kptfile content as part of render result unless using kpt with version under 1.0.0-beta.15 which is less preferred, this actually breaks krm function specs, and will cause kubectl apply -f - fail if piped with the render result.
- support parameterizations for all renders
  - kustomize
    - as kustomize build erases comments in the source manifests and we don't want to change users' source manifests either, we mirror kustomize directory to temporary folder and do transformation(apply-setters) in a imperative way, then run kustomize build against the transformed manifests.
  - helm
    - using helm native support approach --set flag
    - This is not working if the render config is not directly set for manifests.helm,
  - kubectl
  - kpt
    - no extra folder, use kpt render command and output unwrapped result into memory, then running transformers against the output.
    - This currently doesn't work if kpt version is above 1.0.0-beta.15, will fix that in another pr.

Manual Test

add kpt setters comment in your manifests, run skaffold render --set key1=value1 --set key2=value2, should be able to see values with kpt setter comments are replaced by provided values from command line.

codecov · 2023-02-03T15:16:07Z

Codecov Report

Merging #8365 (ced8397) into main (290280e) will decrease coverage by 5.29%.
The diff coverage is 54.41%.

@@            Coverage Diff             @@
##             main    #8365      +/-   ##
==========================================
- Coverage   70.48%   65.20%   -5.29%     
==========================================
  Files         515      607      +92     
  Lines       23150    30100    +6950     
==========================================
+ Hits        16317    19626    +3309     
- Misses       5776     8999    +3223     
- Partials     1057     1475     +418

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/completion.go	`13.04% <0.00%> (-1.25%)`	⬇️
cmd/skaffold/app/cmd/config/list.go	`65.21% <ø> (ø)`
cmd/skaffold/app/cmd/config/set.go	`88.72% <ø> (ø)`
cmd/skaffold/app/cmd/config/util.go	`54.28% <ø> (ø)`
cmd/skaffold/app/cmd/credits.go	`100.00% <ø> (ø)`
cmd/skaffold/app/cmd/credits/export.go	`0.00% <0.00%> (ø)`
cmd/skaffold/app/cmd/deploy.go	`40.90% <0.00%> (-12.94%)`	⬇️
cmd/skaffold/app/cmd/generate_pipeline.go	`60.00% <ø> (ø)`
cmd/skaffold/app/cmd/inspect_modules.go	`65.00% <ø> (ø)`
cmd/skaffold/app/cmd/inspect_profiles.go	`66.66% <ø> (ø)`
... and 419 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aaron-prindle · 2023-02-03T06:48:55Z

 metadata:
  name: getting-started
+  labels:
+    a: hhhh # kpt-set: ${hello}


nit: hhhh -> will-be-overwritten

aaron-prindle · 2023-02-03T07:09:00Z

 		args = append(args, "-f", exp)
 	}
+
+	for _, k := range maps.SortKeys(manifestOverrides) {


if there is a duplicate key set via r.SetValues above, will the key appended later win out? Just want to make sure the key:values set in manifestOverrides have priorty over setValues key:values

yes.. tested locally later write wins.

aaron-prindle · 2023-02-03T07:15:16Z

 var (
 	KptVersion                      = currentKptVersion
-	maxKptVersionAllowedForDeployer = "1.0.0-beta.13"
+	maxKptVersionAllowedForDeployer = "1.0.0-beta.24"


My understanding is that for the kpt renderer any version could be used but for the kpt deployer they broke some things we depend on after 1.0.0-beta.13. @renzodavid9 did you have any additional information here? I'm not sure if we can change this. I think it makes sense to allow any version if kpt deploy isn't required (eg: render only) but I think that is the case currently?

This is a temporary workaround as using 1.0.0-beta.15 above will break kpt render, but using lower version kpt doesn't support running krm function to modify yaml out-place. I'm going to remove the validation and fix kpt render output Kptfile issue in another pr to fix #8306

aaron-prindle · 2023-02-03T18:40:52Z

-			t.CheckFileExistAndContent(filepath.Join(tmpDirObj.Root(), constants.DefaultHydrationDir, kptfile.KptFileName),
-				[]byte(test.updatedKptfile))
-		})
+		t.Skip("todo:" + test.description)


Is this t.Skip necessary here? Might make sense to remove or if it is necessary, add a comment as to why this is skipped

ohh.. I was thinking to re-write the tests. This skip was meant to add comments. I'll add a separate comment.

Just realized that related tests are added in this pr .. I'm removing the test here.

aaron-prindle · 2023-02-07T20:53:05Z

 		deployConfig := pipeline.Deploy

-		if renderConfig.Validate != nil || renderConfig.Transform != nil || renderConfig.Kpt != nil {
+		if renderConfig.Kpt != nil {


Why is this conditional different now?

Why is this conditional different now?

yeah.. the function was mainly used to determine if a kpt hydrationDir should be created , because the original implementation around transform/validate is to marshal related config into a kptfile and put the kptfile and intermediary manifests into the hydrationDir and then call kpt render to do the actual transformation in-place in that direcotry, then call kpt source to get the result into memory. In the new flow, we run krm function against a manifest input stream with kpt fn eval, we don't create any directories. Also, kpt here for renderConfig.Transform and renderConfig.Validate is a krm function executor, this can be replaced by internal function or kustomize run fn. This is not tied to kpt render or kpt live apply , as the method name suggested isKptRendererOrDeployerUsed, those conditions shouldn't be here.

aaron-prindle · 2023-02-08T22:43:47Z

 	}
-	// TODO(tejaldesai) consult with cloud deploy team if namespaces can be set in offline mode
-	// in case namespace is set on the skaffold render cli command.
+	// This is kind of strange, it seems to indicate namespace setting is controlled by this offline flag. If a user set namespace through command line, the command line setting should always be honored


nit: I'm a bit confused if this comment should be here, it seems more this statement should be tracked in an issue - I think it might already be here:
#8318

I don't think this comment adds clarify to any of the surrounding code atm

great! I'll remove it.

aaron-prindle · 2023-02-08T22:46:06Z

+	if sUtil.IsURL(path) {
+		return nil
+	}
+	// todo making everything absolute path


nit: todos usually follow a syntax like
// TODO(#issue-number) <explanation>
OR
// TODO(@ gh-username) <explanation>

aaron-prindle

LGTM, just a few nits. Exciting!

pull-request-size Bot added the size/L label Jan 25, 2023

ericzzzzzzz marked this pull request as draft January 26, 2023 07:03

pull-request-size Bot added size/XL and removed size/L labels Jan 26, 2023

ericzzzzzzz changed the title ~~(WIP)Poc parameterization~~ Jan 26, 2023

pull-request-size Bot added size/XXL and removed size/XL labels Jan 31, 2023

ericzzzzzzz force-pushed the poc-parameterization branch from cb6024e to e1ac0a1 Compare February 2, 2023 19:07

ericzzzzzzz changed the title ~~(WIP)Poc parameterization [no ci]~~ Feb 2, 2023

ericzzzzzzz added 20 commits February 3, 2023 11:03

chore: rebase and resolve confilicts

537112e

..

68328fe

..

897bf4e

chore: fix flag

f1ebd74

base impl for mirroring

e87a6d3

...

533c210

...

08c189b

get set working

b268e6e

support kubectl parameterization

76699e0

parameterization for helm

b22ba24

chore: initialize kube render when remote manifest is set

e800e1c

fix directory issue

c49eb54

fix mirror path issue

1eacc39

..

42a564f

...

750724d

get kpt working

3c27f1e

ref

1c2bb62

refactor kustomize

295c87b

...

641b0af

refoctor

e16cca0

ericzzzzzzz added 5 commits February 3, 2023 11:03

fix tests

4176811

fix tests

072be1a

boilerplate

a7689b4

add manual for --set

004f54d

add kpt-render example in integration

2721c1b

ericzzzzzzz force-pushed the poc-parameterization branch from 8bd8405 to 2721c1b Compare February 3, 2023 16:07

delete kpt-render example

7491924

ericzzzzzzz mentioned this pull request Feb 3, 2023

support kustomize parameterization when dependencies path is absolute #8406

Open

ericzzzzzzz added 5 commits February 6, 2023 00:29

chore: add test

4edd032

chore: nit

dace22c

linters

43ac3a7

linters

5c86c8e

chore: upgrade kpt version

99524d1

aaron-prindle mentioned this pull request Feb 6, 2023

fix: not inject metadata.namespace in manifests rendered with kustomize #8409

Merged

ericzzzzzzz added 3 commits February 6, 2023 13:54

test

5abda44

linters

0d80efb

chore: revert example change

7d5803e

ericzzzzzzz marked this pull request as ready for review February 7, 2023 15:15

aaron-prindle reviewed Feb 7, 2023

View reviewed changes

chore: remove unnecessary tests

386fd54

aaron-prindle reviewed Feb 8, 2023

View reviewed changes

aaron-prindle approved these changes Feb 8, 2023

View reviewed changes

chore: nit

ced8397

ericzzzzzzz added the kokoro:force-run forces a kokoro re-run on a PR label Feb 9, 2023

kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Feb 9, 2023

ericzzzzzzz merged commit c3d15a4 into GoogleContainerTools:main Feb 9, 2023

This was referenced Jun 20, 2023

import apply-setter function to skaffold to replace kpt command call for parameterization #8901

Closed

support parameterization with comments for helm #8912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: parameterization #8365

feat: parameterization #8365
ericzzzzzzz merged 41 commits into
GoogleContainerTools:mainfrom
ericzzzzzzz:poc-parameterization

ericzzzzzzz commented Jan 25, 2023 •

edited

Loading

codecov Bot commented Feb 3, 2023 •

edited

Loading

aaron-prindle Feb 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaron-prindle Feb 3, 2023

ericzzzzzzz Feb 7, 2023

aaron-prindle Feb 3, 2023

ericzzzzzzz Feb 7, 2023 •

edited

Loading

aaron-prindle Feb 3, 2023

ericzzzzzzz Feb 7, 2023

ericzzzzzzz Feb 8, 2023

aaron-prindle Feb 7, 2023 •

edited

Loading

ericzzzzzzz Feb 7, 2023 •

edited

Loading

aaron-prindle Feb 8, 2023

ericzzzzzzz Feb 8, 2023

aaron-prindle Feb 8, 2023 •

edited

Loading

aaron-prindle left a comment

Labels

3 participants

Uh oh!

Conversation

ericzzzzzzz commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Manual Test

codecov Bot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericzzzzzzz Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ericzzzzzzz Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

aaron-prindle left a comment

Choose a reason for hiding this comment

Labels

3 participants

ericzzzzzzz commented Jan 25, 2023 •

edited

Loading

codecov Bot commented Feb 3, 2023 •

edited

Loading

ericzzzzzzz Feb 7, 2023 •

edited

Loading

aaron-prindle Feb 7, 2023 •

edited

Loading

ericzzzzzzz Feb 7, 2023 •

edited

Loading

aaron-prindle Feb 8, 2023 •

edited

Loading