configmap with alphabetical order #5334
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[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 |
charles-chenzz
force-pushed
the
configmap_alphabetical
branch
from
d118fe2
to
e0640ed
Compare
|
/cc |
There was a problem hiding this comment.
Please add tests demonstrating that ConfigMap items get outputed in alphabetical order. You can probably add them to api/krusty, maybe in this file: https://github.com/kubernetes-sigs/kustomize/blob/master/api/krusty/configmaps_test.go. You can use the contents from this comment #4292 (comment) as input to the test. Thanks!
| for k := range m { | ||
| keys[i] = k | ||
| i++ | ||
| keys = append(keys, k) |
There was a problem hiding this comment.
I don't think this code change is a functional change, it looks like it does exactly the same thing functionally as it did previously, but I believe your version has slightly worse performance because we are appending to an array rather than having a prebuilt array.
You can test this out in go playground and see that it functionally does the same thing: https://go.dev/play/p/1BBr3XTnHBS
There was a problem hiding this comment.
I've check the link and the functional did do the same things... the performance should be relativity same as we pre-allocate capacity but I didn't test it so I can't guarantee.
the configmap data is getting sort and loaded here, but I don't know where it get un-sorted when return to the user..
|
/uncc |
|
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 |
k8s-ci-robot
added
the
lifecycle/stale
|
Hi @charles-chenzz, are you planning to continue this PR? Let's add some test to validate your implementation |
don't have much bandwidth recently.. If you want to take it on please feel free to do so! @antoooks |
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
|
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 |
k8s-ci-robot
added
the
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 |
k8s-ci-robot
added
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-sigs/prow repository. |
this PR tend to resolve #4292 by using alphabetical way. as I currently can't figure out a better way to move #5308 on.
I used to have a scenes that need to sort keys order in a map, and I've done it like here. IMHO, the configmap key is getting sort here and then get load.