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

Example of configuring builtin plugin. #1534

Merged
merged 1 commit into from
Sep 17, 2019

Conversation

monopole
Copy link
Contributor

Requires #1532

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2019
@monopole
Copy link
Contributor Author

@richardmarshall @jbrette PTAL

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2019
@monopole monopole force-pushed the configExample branch 2 times, most recently from 9a50056 to 44c0e74 Compare September 17, 2019 21:03
docs/plugins/builtins.md Outdated Show resolved Hide resolved
docs/plugins/builtins.md Outdated Show resolved Hide resolved
docs/plugins/builtins.md Outdated Show resolved Hide resolved
> apiVersion: builtin
> kind: ImageTagTransformer
> metadata:
> name: notImportantHere
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually really important. In order to be able to change two images, you need to add multiple transformers. The name of the transformers have to be different, otherwise the resources accumulation will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to "not-imporant-in-this-example" which is what i meant.
indeed, the names need to be different if using more than one.
maybe just change it to a my-image-transformer or something?

> kind: NamespaceTransformer
> metadata:
> name: notImportantHere
> namespace: test
Copy link
Contributor

Choose a reason for hiding this comment

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

The syntax of the namespace transformer is kind of misleading. the "metadata/name" is not important but the "metadata/namespace" is. Unlike the other transformer (prefix, suffix), that field is not part of the "spec" of the Transformer but part of the metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not every plugin has the config under spec. Even for prefix/suffix transformer, it's prefix and suffix instead of spec. The transformer/generator plugin read the whole k8s styled yaml as its configuration, not only spec field.

Copy link
Contributor

@jbrette jbrette Sep 17, 2019

Choose a reason for hiding this comment

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

Let me rephrase. All the other transformers have the addditional parameters at the top level....basically the fieldSpecs level, the metadata field is actually irrelevant most of the time.

apiVersion: builtin
kind: LabelTransformer
metadata:
  name: notImportantHere
labels:
  app: myApp
  env: production
fieldSpecs:
  - path: metadata/labels
    create: true

or

apiVersion: builtin
kind: PrefixSuffixTransformer
metadata:
  name: notImportantHere
prefix: baked-
suffix: -pie
fieldSpecs:
  - path: metadata/name

But not for the namespace transformer. The consistent way of doing, should be like this:

apiVersion: builtin
kind: NamespaceTransformer
metadata:
  name: notImportantHere
namespace: test
fieldSpecs:
  - path: metadata/namespace

The other issue is here. The name is actually important for ImageTransformer if you need multiple image transformations. The metadata/namespace is supposed to be completely irrelevant but is not, and the namespace transformer ends up being in a different namespace than all the other transformers. This will be an issue when we will try to apply kustomize on top of the transformer config.yaml

func (kt *KustTarget) configureExternalTransformers() ([]resmap.Transformer, error) {
	ra := accumulator.MakeEmptyAccumulator()
	err := kt.accumulateResources(ra, kt.kustomization.Transformers)
	if err != nil {
		return nil, err
	}
	return kt.pLdr.LoadTransformers(kt.ldr, ra.ResMap())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbrette yes, the namespace transformer is an outlier.
see comments here: #1280
it's philosophically wrong, but in practice is works and allows for a smaller config file, fwiw.
we should probably 'fix' it if for no other reason than to avoid this kind of confusion/conversation.
Not in this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this clarification, and for this comment especially: #1280 (comment)!

@monopole monopole force-pushed the configExample branch 6 times, most recently from dc81757 to 7408c8c Compare September 17, 2019 23:08
@monopole
Copy link
Contributor Author

OK comments addressed. sorry was doing a bunch of pushes to see how it looked since my local md renderer is busted and too lazy to fix at the moment

@monopole
Copy link
Contributor Author

I'm not happy with the section headings, e.g.

SecretGenerator

Usage via kustomization.yaml

field name: secretGenerator

Usage via plugin

Arguments

Example

could use some improvement, but i'd rather fix in the context of a code generator since it's so tedious to do this.

@Liujingfang1
Copy link
Contributor

@monopole Thank you for this work.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2019
@k8s-ci-robot k8s-ci-robot merged commit a3103f1 into kubernetes-sigs:master Sep 17, 2019
@monopole monopole deleted the configExample branch September 18, 2019 01:46
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants