This repository has been archived by the owner on Oct 20, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 125
Rename HTTPRoutes HTTPRoute #30
Merged
nicholasjackson
merged 4 commits into
servicemeshinterface:master
from
nicholasjackson:rename_httproutes
May 10, 2019
Merged
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
f375f9a
Rename HTTPRoutes HTTPRoute
nicholasjackson 8f6a88e
Rename HTTPRoutes something more sane
nicholasjackson 0f1ccc5
update comments for IdentityBinding and ClusterBinding
nicholasjackson aaef7d5
Added namespace to allow selection from CRD
nicholasjackson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Both target and binding aren't namespaced (in my head at least).
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.
Aha good catch, so the reason I added that is when we are building the CRD and are actually writing the code to read the TrafficTarget, the KubeAPI client requires the object namespace as a parameter.
Originally I added namespace to the TargetRef so that you could do the lookup, then reverted that as IdentityBindings would always be in the same location as TrafficTargets. However, that assumption does not apply for ClusterIdentityBinding.
I think we need to add namespace to the ref then?
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.
It looks like
RoleBinding
andClusterRoleBinding
don't need the namespace. I suspect that is because you can onlytargetRef
the cluster or non-cluster resource. I think thattargetRef
is a custom object there though (and not a ObjectReference). You're in the code so I'll defer to you though.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 we need namespace on everything, it will just make things easier when coding the CRDs no assumptions. I have added this in the latest commit. We can always change things later to make things easier.