Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Retain field order after running any arbitrary functions on resources #4021

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Retain field order after running any arbitrary functions on resources #4021

merged 4 commits into from
Jul 7, 2021

Conversation

phanimarupaka
Copy link
Contributor

@phanimarupaka phanimarupaka commented Jun 30, 2021 •
edited
Loading

Field order might be altered due to round-tripping in arbitrary functions. Similar to CopyComments logic, this PR aims at syncing the field ordering of transformed resource with the input resource.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 30, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: phanimarupaka

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 30, 2021
@phanimarupaka phanimarupaka requested review from droot and removed request for justinsb and pwittrock June 30, 2021 18:32
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2021
@phanimarupaka phanimarupaka changed the title [WIP] Retain field order after running any arbitrary functions on resources Retain field order after running any arbitrary functions on resources Jul 1, 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 Jul 1, 2021
natasha41575
natasha41575 reviewed Jul 2, 2021
View reviewed changes
Copy link
Contributor

@natasha41575 natasha41575 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Change looks fine to me so far, thank you for working on this! Some small comments.

@k8s-ci-robot
Copy link
Contributor

@phanimarupaka: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@phanimarupaka phanimarupaka mentioned this pull request Jul 5, 2021
Closed
@natasha41575 natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 7, 2021
natasha41575
natasha41575 reviewed Jul 7, 2021
View reviewed changes
kyaml/order/syncorder_test.go
name: after
spec:
containers:
# ports comment
Copy link
Contributor

@natasha41575 natasha41575 Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would have expected this comment should come right before ports: since that's where it is in the input. Do you think that makes sense, and is it possible to do?

EDIT: Never mind, I see two scenarios:

  1. The comment is specifically talking about the ports field
  2. The comment is talking about the entire sequence node.

I guess it will be impossible to know which case the user intends, so we can leave this as is.

@natasha41575
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2021
@k8s-ci-robot k8s-ci-robot merged commit e1804cb into kubernetes-sigs:master Jul 7, 2021
@phanimarupaka phanimarupaka mentioned this pull request Jul 7, 2021
Open
6 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@natasha41575 natasha41575 natasha41575 left review comments

@droot droot Awaiting requested review from droot

@frankfarzan frankfarzan Awaiting requested review from frankfarzan

@mortent mortent Awaiting requested review from mortent

@KnVerey KnVerey Awaiting requested review from KnVerey

Assignees

@natasha41575 natasha41575

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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phanimarupaka @k8s-ci-robot @natasha41575