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

Opt-out the kustomize feature via go build tags #299

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Jan 28, 2023

Problems:

  • The k-d-p supports a kustomize hydration feature. This feature requires library kustomize/api. This library itself is commonly used by other libraries and users can encounter dependency conflicts if the other library uses a different kustomize/api release (what happened on my side and I cannot bump the other kustomize/api version).
  • The k-d-p kustomize feature itself potentially causes friction issues, like fs operation, run kustomize before other transformation operations.

Description:

This PR split the kustomize hydration to 1. a generic kustomize build and 2. a no-op in different files; It adds a go build constraint without_kustomize to give users the ability to switch to opt out the kustomize feature in build time and no need to change their source code.

Test:
This PR is tested using the guestbook example by changing the make run for the following two cases

  1. Preserve existing feature
make run

output

2023-01-27T23:29:57-08:00	INFO	running kustomize	{"controller": "guestbook-controller", "object": {"name":"guestbook-sample","namespace":"default"}, "namespace": "default", "name": "guestbook-sample", "reconcileID": "fbcb7051-36c0-4358-aafc-5c84c6b96e2a", "kustomizer": "EnabledKustomize"}
  1. Skip kustomize
# Makefile
run: manifests generate fmt vet ## Run a controller from your host.
	go run -tags without_kustomize ./main.go
make run

output

2023-01-27T23:38:37-08:00	INFO	skip running kustomize	{"controller": "guestbook-controller", "object": {"name":"guestbook-sample","namespace":"default"}, "namespace": "default", "name": "guestbook-sample", "reconcileID": "38da0981-8143-43e5-96a4-7e551018758f", "kustomizer": "SkipKustomize"}

UPDATED

go run -tags without_kustomize ./main.go

...
2023-01-28T23:16:16-08:00	ERROR	building deployment objects	{"controller": "guestbook-controller", "object": {"name":"guestbook-sample","namespace":"default"}, "namespace": "default", "name": "guestbook-sample", "reconcileID": "b62ee9fb-be37-4094-9bb7-c3cce756c03e", "error": "error running kustomize: kustomize support is not compiled in (built with tag `without_kustomize`)"}
go run ./main.go

2023-01-28T23:19:46-08:00	INFO	running kustomize	{"controller": "guestbook-controller", "object": {"name":"guestbook-sample","namespace":"default"}, "namespace": "default", "name": "guestbook-sample", "reconcileID": "eb7c1f04-771e-45a1-b44f-d1b451357d46", "manifestPath": "channels/packages/guestbook/0.0.1"}

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 28, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 28, 2023
func (k *SkipKustomize) Run(ctx context.Context, _ filesys.FileSystem, _ string) ([]byte, error) {
log := log.FromContext(ctx)
log.WithValues("kustomizer", "SkipKustomize").Info("skip running kustomize")
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could return an error here, like fmt.Errorf("kustomize support is not compiled in (built with tag without_kustomize)")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the error message. Updated

type EnabledKustomize struct{}

// Run calls the kustomize/api library to run `kustomize build`.
func (k *EnabledKustomize) Run(ctx context.Context, fs filesys.FileSystem, manifestPath string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI you can have the same function with a build tag and without the build tag.

So you could declare RunKustomize in both.

One would have the build-tag //go:build without_kustomize, the other would have the build tag //go:build !without_kustomize.

Not a blocker though - and the refactoring is nice - but may be simpler in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! I'm trying to use function/struct name to make the code self-explained. But I agree the EnabledKustomize and SkipKustomize structs may give duplicate info since the go file name ("disabled" "enabled) and the build tags have already explained the rationale. Updated!

@yuwenma yuwenma requested review from justinsb and removed request for johnsonj January 30, 2023 19:20
@yuwenma
Copy link
Contributor Author

yuwenma commented Jan 30, 2023

/assign @justinsb

@justinsb
Copy link
Contributor

Thanks @yuwenma!

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, yuwenma

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit c58a777 into kubernetes-sigs:master Jan 31, 2023
@atoato88
Copy link
Contributor

LGTM 😄
Should we add documentation about without_kustomize tag?

@yuwenma
Copy link
Contributor Author

yuwenma commented Jan 31, 2023

@atoato88 that's a good idea. I updated the doc here #301 Please let me know if there's anywhere else I shall update.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants