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.

Sign up for GitHub

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

Jump to bottom

replacement transformer that uses the new replacement filter #3737

Merged
merged 1 commit into from
Apr 22, 2021
Merged

replacement transformer that uses the new replacement filter #3737

merged 1 commit into from
Apr 22, 2021

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Mar 18, 2021 •
edited
Loading

Part of #3492.

This PR implements a basic builtin replacement transformer that is connected to a replacements field in the kustomization file.

ALLOW_MODULE_SPAN

@natasha41575 natasha41575 requested a review from monopole March 18, 2021 21:53
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 18, 2021
@natasha41575 natasha41575 requested review from KnVerey and removed request for Liujingfang1 March 18, 2021 21:54
@natasha41575 natasha41575 changed the title Add replacement transformer plugin that uses replacement filter add replacement transformer plugin that uses replacement filter Mar 18, 2021
@natasha41575 natasha41575 mentioned this pull request Mar 18, 2021
Merged
@natasha41575 natasha41575 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2021
@natasha41575 natasha41575 marked this pull request as draft March 18, 2021 23:44
@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch from 92096cc to b239e3a Compare March 19, 2021 21:09
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2021
@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch from b239e3a to eddb9fd Compare April 1, 2021 22:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2021
@natasha41575 natasha41575 changed the title add replacement transformer plugin that uses replacement filter replacement transformer POC that uses the new replacement filter Apr 1, 2021
@natasha41575 natasha41575 marked this pull request as ready for review April 1, 2021 22:57
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2021
@natasha41575 natasha41575 requested a review from KnVerey April 1, 2021 22:58
KnVerey
KnVerey reviewed Apr 2, 2021
View reviewed changes
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

A general question about release logistics: Unless there's an additional step I'm not aware of, it seems like this will expose the new field in the first release we make after merging. Given that, should this PR include docs changes (e.g. in examples/)? Were we deferring the additional testing, and the rejection and field options implementations to a follow-up PR or potentially until a follow-up release?

api/internal/target/kusttarget_configplugin.go Outdated Show resolved Hide resolved
plugin/builtin/replacementtransformer/ReplacementTransformer.go Outdated Show resolved Hide resolved
@natasha41575
Copy link
Contributor Author

natasha41575 commented Apr 2, 2021 •
edited
Loading

@KnVerey Regarding your general questions I gave it some thought and I think you're right. I'll comment out exposing a replacements field in the kustomization file. It'll be better to add that with documentation in a later PR after everything else is in place.

With this PR we can focus on the plugin itself and the other concerns you've brought up.

@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch from eddb9fd to 68d3f36 Compare April 2, 2021 19:28
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2021
@natasha41575 natasha41575 changed the title replacement transformer POC that uses the new replacement filter WIP: replacement transformer POC that uses the new replacement filter Apr 14, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2021
@natasha41575 natasha41575 changed the title WIP: replacement transformer POC that uses the new replacement filter WIP: replacement transformer that uses the new replacement filter Apr 14, 2021
@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch from 68d3f36 to 3f3db4f Compare April 14, 2021 23:02
@k8s-ci-robot k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 14, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575

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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 14, 2021
@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch 2 times, most recently from db9bb26 to 9c06987 Compare April 14, 2021 23:09
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2021
@natasha41575 natasha41575 force-pushed the ReplacementTransformer branch from 9c06987 to 038bc77 Compare April 15, 2021 21:12
@natasha41575 natasha41575 changed the title WIP: replacement transformer that uses the new replacement filter replacement transformer that uses the new replacement filter Apr 15, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2021
@natasha41575 natasha41575 requested a review from KnVerey April 19, 2021 22:41
@monopole
Copy link
Contributor

/lgtm

We can tweak the ResMap api a bit to eliminate all those duplicated loops over resources that appear in plugins.
I'll do that, and remove the Resource.Node() method this PR adds.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021
@monopole monopole merged commit 06add3a into kubernetes-sigs:master Apr 22, 2021
monopole
monopole reviewed Apr 22, 2021
View reviewed changes
Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

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

/lgtm

@natasha41575
Copy link
Contributor Author

@monopole thanks! I'll make documentation PRs when this is released

@natasha41575 natasha41575 deleted the ReplacementTransformer branch April 22, 2021 18:33
@monopole monopole mentioned this pull request Apr 28, 2021
Closed
@SGStino SGStino mentioned this pull request Aug 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monopole monopole monopole left review comments

@KnVerey KnVerey Awaiting requested review from KnVerey

Assignees

@monopole monopole

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@natasha41575 @k8s-ci-robot @monopole @KnVerey