Add comment pointing out incorrect SDN annotation naming #12627
Merged
Conversation
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
|
We're starting to do a better job of qualifiying these, but all annotations
should be bounded by their group - so APIs that are under the openshift
networking group would be "network.openshift.io", with a prefix on that for
the high level grouping. I would recommend "
namespace.network.openshift.io/*" for your suggested name (we're having a
discussion about how to enforce this across all Kube projects, and group
will be the common one).
…On Mon, Jan 23, 2017 at 2:39 PM, Dan Winship ***@***.***> wrote:
In openshift/openshift-sdn#271
<openshift/openshift-sdn#271> we added the
pod.network.openshift.io/assign-macvlan annotation, with the name based
on comments from @smarterclayton <https://github.com/smarterclayton>
(specifically openshift/openshift-sdn#271 (comment)
<openshift/openshift-sdn#271 (comment)>
).
Two later annotations copied the pod.network.openshift.io/ part despite
being HostSubnet annotations rather than Pod annotations. So it seems to me
that those names were wrong, and while we can't fix them now (because API)
we can at least clearly note that they're wrong so that people don't keep
copying the wrongness.
I argue in the comment here that they should have the prefix
hostsubnet.openshift.io/, because they're HostSubnet annotations, and
because including network in there like with the macvlan annotation would
be redundant, since HostSubnet, unlike Pod, is inherently
networking-related. But then maybe it would be better with network, just
for consistently namespacing sdn-related annotations?
(This came up while figuring out what to call the NetNamespace annotation
for enabling multicast in the namespace, which, FTR, I'm planning to call
netnamespace.openshift.io/multicast-enabled, by the same logic.)
@smarterclayton <https://github.com/smarterclayton> does this match your
view of how annotation names should work?
------------------------------
You can view, comment on, or merge this pull request online at:
#12627
Commit Summary
- Add comment pointing out incorrect SDN annotation naming
File Changes
- *M* pkg/sdn/api/plugin.go
<https://github.com/openshift/origin/pull/12627/files#diff-0> (5)
- *M* pkg/sdn/plugin/controller.go
<https://github.com/openshift/origin/pull/12627/files#diff-1> (2)
- *M* pkg/sdn/plugin/subnets.go
<https://github.com/openshift/origin/pull/12627/files#diff-2> (6)
Patch Links:
- https://github.com/openshift/origin/pull/12627.patch
- https://github.com/openshift/origin/pull/12627.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12627>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5Bm_K2tdVk5KBgDjBkK_kuRunWsks5rVQH3gaJpZM4LrdKq>
.
|
|
And by group I mean - if your annotation only applies to something by
openshift sdn (which is "network.openshift.io") or a resource that lives
within that group, it would have the prefix. If it was for an unrelated
networking project within openshift we'd assign a new top level identifier.
On Mon, Jan 23, 2017 at 5:30 PM, Clayton Coleman <[email protected]>
wrote:
… We're starting to do a better job of qualifiying these, but all
annotations should be bounded by their group - so APIs that are under the
openshift networking group would be "network.openshift.io", with a prefix
on that for the high level grouping. I would recommend "
namespace.network.openshift.io/*" for your suggested name (we're having a
discussion about how to enforce this across all Kube projects, and group
will be the common one).
On Mon, Jan 23, 2017 at 2:39 PM, Dan Winship ***@***.***>
wrote:
> In openshift/openshift-sdn#271
> <openshift/openshift-sdn#271> we added the
> pod.network.openshift.io/assign-macvlan annotation, with the name based
> on comments from @smarterclayton <https://github.com/smarterclayton>
> (specifically openshift/openshift-sdn#271 (comment)
> <openshift/openshift-sdn#271 (comment)>
> ).
>
> Two later annotations copied the pod.network.openshift.io/ part despite
> being HostSubnet annotations rather than Pod annotations. So it seems to me
> that those names were wrong, and while we can't fix them now (because API)
> we can at least clearly note that they're wrong so that people don't keep
> copying the wrongness.
>
> I argue in the comment here that they should have the prefix
> hostsubnet.openshift.io/, because they're HostSubnet annotations, and
> because including network in there like with the macvlan annotation
> would be redundant, since HostSubnet, unlike Pod, is inherently
> networking-related. But then maybe it would be better with network, just
> for consistently namespacing sdn-related annotations?
>
> (This came up while figuring out what to call the NetNamespace annotation
> for enabling multicast in the namespace, which, FTR, I'm planning to call
> netnamespace.openshift.io/multicast-enabled, by the same logic.)
>
> @smarterclayton <https://github.com/smarterclayton> does this match your
> view of how annotation names should work?
> ------------------------------
> You can view, comment on, or merge this pull request online at:
>
> #12627
> Commit Summary
>
> - Add comment pointing out incorrect SDN annotation naming
>
> File Changes
>
> - *M* pkg/sdn/api/plugin.go
> <https://github.com/openshift/origin/pull/12627/files#diff-0> (5)
> - *M* pkg/sdn/plugin/controller.go
> <https://github.com/openshift/origin/pull/12627/files#diff-1> (2)
> - *M* pkg/sdn/plugin/subnets.go
> <https://github.com/openshift/origin/pull/12627/files#diff-2> (6)
>
> Patch Links:
>
> - https://github.com/openshift/origin/pull/12627.patch
> - https://github.com/openshift/origin/pull/12627.diff
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#12627>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABG_p5Bm_K2tdVk5KBgDjBkK_kuRunWsks5rVQH3gaJpZM4LrdKq>
> .
>
|
Also document other SDN annotations, and fix up a constant name to match conventions.
danwinship
force-pushed
the
annotations
branch
from
a3d888f to
1b59b71
Compare
It's an annotation on the NetNamespace object, not the Namespace though, so maybe "netnamespace.network.openshift.io" would be best? (That's about the next PR though. For this PR, it sounds like HostSubnet annotations should definitely be "hostsubnet.network.openshift.io/", so I've updated the comment accordingly. OK @openshift/networking ?) |
knobunc
approved these changes
Jan 23, 2017
|
[merge] |
|
Evaluated for origin merge up to 1b59b71 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13303/) (Base Commit: 7b6ccb0) (Image: devenv-rhel7_5777) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In openshift/openshift-sdn#271 we added the
pod.network.openshift.io/assign-macvlanannotation, with the name based on comments from @smarterclayton (specifically openshift/openshift-sdn#271 (comment)).Two later annotations copied the
pod.network.openshift.io/part despite being HostSubnet annotations rather than Pod annotations. So it seems to me that those names were wrong, and while we can't fix them now (because API) we can at least clearly note that they're wrong so that people don't keep copying the wrongness.I argue in the comment here that they should have the prefix
hostsubnet.openshift.io/, because they're HostSubnet annotations, and because includingnetworkin there like with the macvlan annotation would be redundant, since HostSubnet, unlike Pod, is inherently networking-related. But then maybe it would be better withnetwork, just for consistently namespacing sdn-related annotations?(This came up while figuring out what to call the NetNamespace annotation for enabling multicast in the namespace, which, FTR, I'm planning to call
netnamespace.openshift.io/multicast-enabled, by the same logic.)@smarterclayton does this match your view of how annotation names should work?