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

Removing scope fields from object references #882

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Removing scope fields from object references #882

merged 1 commit into from
Sep 28, 2021

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind cleanup
/kind api-change

What this PR does / why we need it:
Our object references use kind instead of resource which requires implementations to have a predefined kind->resource mapping. That means there can never be ambiguity within an implementation as far as whether a reference is to a cluster-scoped or namespace-scoped resource.

This also removes the ability for ParentRef to refer to cluster-scoped resources. Although we can add that in the future with a docs update. There do not appear to be any use cases for that today.

Which issue(s) this PR fixes:
Fixes #838

Does this PR introduce a user-facing change?:

* The `scope` field has been removed from all object references.
* ParentRefs can no longer refer to cluster-scoped resources.

@robscott
Removing scope fields from object references
7314699 
Our object references use kind instead of resource which requires
implementations to have a predefined kind->resource mapping. That means
there can never be ambiguity within an implementation as far as whether
a reference is to a cluster-scoped or namespace-scoped resource.

This also removes the ability for ParentRef to refer to cluster-scoped
resources. Although we can add that in the future with a docs update,
there do not appear to be any use cases for that today.
@robscott robscott added this to the v1alpha2 milestone Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added 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. labels Sep 27, 2021
@robscott
Copy link
Member Author

Adding a hold until we have consensus on this.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
howardjohn
howardjohn approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

🎉

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, robscott

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

@youngnick
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 Sep 27, 2021
@robscott
Copy link
Member Author

Follow up: @hbagdi found the original discussion that resulted in the introduction of scope: kubernetes/enhancements#2366 (comment). Although we were concerned that the initial rationale may have been broader than ambiguity, it seems like that really was the primary concern.

@youngnick
Copy link
Contributor

That comment does make clear that what we're doing for GatewayClass is not preferred, although personally, I'm okay with it in this case.

@hbagdi
Copy link
Contributor

hbagdi commented Sep 28, 2021

/lgtm

@robscott
Copy link
Member Author

Discussed at community meeting yesterday as well, it seems like there's pretty broad consensus on this change.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit a015247 into kubernetes-sigs:master Sep 28, 2021
@robscott robscott deleted the no-scope branch January 8, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

@bowei bowei Awaiting requested review from bowei

@youngnick youngnick Awaiting requested review from youngnick

Assignees

@hbagdi hbagdi

@youngnick youngnick

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1alpha2
Development

Successfully merging this pull request may close these issues.

Scope in object references may be unnecessary
5 participants
@robscott @k8s-ci-robot @youngnick @hbagdi @howardjohn