Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Remove all metadata fields but labels from targets of kf federate #1086

Merged
merged 1 commit into from
Aug 16, 2019
Merged

Remove all metadata fields but labels from targets of kf federate #1086

merged 1 commit into from
Aug 16, 2019

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 7, 2019

Previously kubefedctl federate removed a subset of metadata fields from the resource targeted for conversion to a federated resource. It wasn't removing metadata.annotations or metadata.finalizers despite those fields not being supported for propagation. This change ensures that all metadata fields are removed except labels to ensure that fields set by the local cluster and its controllers are not included in the template of the new federated resource.

@marun marun added this to the v0.1.0 milestone Aug 7, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2019
@xunpan
Copy link
Contributor

xunpan commented Aug 9, 2019

@marun
For annotation, I'm not sure if it is functional right to remove it from resource target.
If I annotate my object and then federate it across cluster, the annotation lost in other clusters for the objects. This seems not exepcted.

@marun
Copy link
Contributor Author

marun commented Aug 9, 2019

@xunpan Please see #1010. Annotations can no longer be set directly in the template of a federated resource due to the potential for conflict with controllers in local clusters. It's only possible to set annotations via override. To your point, though, it would make sense to log a warning indicating that annotations are being stripped so a user can take action to manually create overrides if desired.

@marun
Copy link
Contributor Author

marun commented Aug 12, 2019

Updated to warn when annotations are present on the target resource.

Copy link
Contributor

@xunpan xunpan left a comment

Choose a reason for hiding this comment

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

LGTM

Previously `kubefedctl federate` removed a subset of metadata fields
from the resource targeted for conversion to a federated resource. It
wasn't removing annotations or finalizers despite those fields not
being supported for propagation. This change ensures that all fields
are removed except labels to ensure that fields set by the local
cluster and its controllers are not included in the template of the
new federated resource.
@marun
Copy link
Contributor Author

marun commented Aug 15, 2019

Rebased

Copy link
Contributor

@font font left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit 7050eb0 into kubernetes-retired:master Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants