-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
configmap item with order #5308
configmap item with order #5308
Conversation
This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: charles-chenzz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…eive changes in interface Generate
…n interface Generate
kustomize/command/edit api/testutil/kusttest plugin/example.someteam.com receive changes arg in the function
the lint checks failed, but wasn't about the mismatch of args. have no clues why it would failed. would defer till I fix the Test Linux and Test MacOS CI jobs.
|
|
62f17ca
to
92f1f86
Compare
I convert to ready for review for gathering some helps and reviews |
/cc @stormqueen1990 |
ac9b4ac
to
73e762a
Compare
73e762a
to
206afc7
Compare
206afc7
to
ce9f793
Compare
ce9f793
to
5675ab9
Compare
@charles-chenzz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, @charles-chenzz! 👋
I left some suggestions. I can also reproduce the error you're seeing about mismatched arguments locally after changing the signature of Generate
in converter_test.go
, so I'm assuming the change in the method signature might require more steps to pass tests. Perhaps @natasha41575 or @annasong20 can give us some help to understand the issue?
} | ||
if err = rn.LoadMapIntoConfigMapData(m); err != nil { | ||
return nil, err | ||
return nil, nil, fmt.Errorf("failed to LoadMapIntoConfigMapData, error:%w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: add a space between the colon and %w
for readability
return nil, nil, fmt.Errorf("failed to LoadMapIntoConfigMapData, error:%w", err) | |
return nil, nil, fmt.Errorf("failed to LoadMapIntoConfigMapData, error: %w", err) |
@@ -206,7 +206,7 @@ immutable: true | |||
for n := range testCases { | |||
tc := testCases[n] | |||
t.Run(n, func(t *testing.T) { | |||
rn, err := MakeConfigMap(kvLdr, &tc.args) | |||
rn, _, err := MakeConfigMap(kvLdr, &tc.args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test should be updated to validate the orderKeys
return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion to add a space between :
and %w
in this file.
@@ -214,7 +214,7 @@ immutable: true | |||
for n := range testCases { | |||
tc := testCases[n] | |||
t.Run(n, func(t *testing.T) { | |||
rn, err := MakeSecret(kvLdr, &tc.args) | |||
rn, _, err := MakeSecret(kvLdr, &tc.args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion as in configmap_test.go
here.
subRa, _, err = subKt.accumulateTarget(ra) | ||
ra = accumulator.MakeEmptyAccumulator() | ||
} else { | ||
// Child Kustomizations create a new accumulator which resolves their kustomization directives, which will later | ||
// be merged into the current accumulator. | ||
subRa, err = subKt.AccumulateTarget() | ||
subRa, _, err = subKt.AccumulateTarget() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was discarding the new output here the goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think these tests should validate the new output in some way.
"configmap %s illegally repeats the key `%s`", name, p.Key) | ||
} | ||
knownKeys[p.Key] = p.Value | ||
orderKeys = append(orderKeys, p.Key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this part. Where is the ordering happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was trying to using a slice of string to preserve the order when for range the pairs, then return it. I was based on the issue and the former PR as my context about this feature. but what I commit in this PR might have details that wrong or misunderstood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be some extra steps needed. Just adding the keys to a slice will likely not be enough to guarantee a predictable order. Another thing I noticed while investigating this code is that the output from this method is discarded, so what ends up being written to the file is exactly the user input:
kustomize/kustomize/commands/edit/add/configmap.go
Lines 114 to 116 in 7c36ed2
args.Options = types.MergeGlobalOptionsIntoLocal( | |
args.Options, k.GeneratorOptions) | |
_, err := rf.MakeConfigMap(ldr, args) |
@@ -30,7 +30,7 @@ type TransformerWithProperties struct { | |||
|
|||
// A Generator creates an instance of ResMap. | |||
type Generator interface { | |||
Generate() (ResMap, error) | |||
Generate() (ResMap, []string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So unfortunately, this is a public interface and we can't change the signature (this is how users built legacy style extensions and the new KRM plugin extensions and we can't break that).
I think it also isn't quite sensical for this Generator to return an array of keys just for the ConfigMap issue. That type of data is specific to the ConfigMap generator and is not generalizable to all generators, so should instead be attached to the ConfigMapGenerator and methods - and only to things that are specific to the ConfigMapGenerator.
A note on the issue is that maintaining the original order of ConfigMaps is not strictly needed. We can also output the ConfigMap items in alphabetical order. As long as the output is deterministic, we can consider the issue as resolved. If there isn't a good way to do this without modifying the generator struct, then we will have to mark the issue as not feasible and explain why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think about other solution to see if we change limit the changes only to configmapgenerator
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
this PR tend to solve #4292
since #5210 remain stagnant, so I took it up.
Also, I borrow some of the code in #5210