Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Add role label to weave containers so scope can filter #1290

Merged
merged 1 commit into from
Aug 27, 2015
Merged

Conversation

tomwilkie
Copy link
Contributor

As part of weaveworks/scope#337, I'd like to label the weave & scope containers and have them filtered out of the scope view by default.

Let the bike shedding begin...

@tomwilkie
Copy link
Contributor Author

feedback from @rade - what happens with older versions of docker? Need to support docker 1.3.1

@tomwilkie
Copy link
Contributor Author

To clarify, role=system is something I just made up.

@tomwilkie
Copy link
Contributor Author

@errordeveloper how do kubernetes do labels? If there an existing schema / topology?

@tomwilkie
Copy link
Contributor Author

@errordeveloper says k8s does something like this:

"Labels": {
    "io.kubernetes.pod.name": "kube-system/kube-dns-v8-zwokj"
}

@tomwilkie
Copy link
Contributor Author

@rade labels seem to be backward compatible (this imports and runs on docker 1.3.1)

@awh
Copy link
Contributor

awh commented Aug 21, 2015

Is there something contentious about this PR? It's a tiny change but has been open for nine days...

@tomwilkie
Copy link
Contributor Author

@MikeB was against the approach of label containers then filtering based on
label.

On Friday, 21 August 2015, Adam Harrison [email protected] wrote:

Is there something contentious about this PR? It's a tiny change but has
been open for nine days...


Reply to this email directly or view it on GitHub
#1290 (comment).

@squaremo
Copy link
Contributor

@MikeB was against the approach of label containers then filtering based on label.

weaveworks/scope#337 (comment):

If you want this to be a general mechanism for hiding containers from display in Scope, I think using labels is a poor choice. Firstly, it's indirect -- you're asking the user to go and change how a container is labelled (which involves recreating the container!), just to hide it in their display. Secondly, the user may not even have control over how containers are created -- that might be done by kubernetes, or marathon, or whatever.

If you want a hook on which to hang some by-default filtering of weave containers, then labels are fine, but you should give them a namespace.

@tomwilkie
Copy link
Contributor Author

f you want a hook on which to hang some by-default filtering of weave containers, then labels are fine, but you should give them a namespace.

Thats exactly what we want - the filtering mechanism with be general, but
we need to filter on something, and we'd like that to be a label. I'll
namespace this one.

On Tue, Aug 25, 2015 at 1:34 PM, Michael Bridgen [email protected]
wrote:

@MikeB https://github.com/mikeb was against the approach of label
containers then filtering based on label.

weaveworks/scope#337 (comment)
weaveworks/scope#337 (comment):

If you want this to be a general mechanism for hiding containers from
display in Scope, I think using labels is a poor choice. Firstly, it's
indirect -- you're asking the user to go and change how a container is
labelled (which involves recreating the container!), just to hide it in
their display. Secondly, the user may not even have control over how
containers are created -- that might be done by kubernetes, or marathon, or
whatever.

If you want a hook on which to hang some by-default filtering of weave
containers, then labels are fine, but you should give them a namespace.


Reply to this email directly or view it on GitHub
#1290 (comment).

@tomwilkie
Copy link
Contributor Author

Failures are unrelated, see #1360

@tomwilkie tomwilkie changed the title Add role=system label to weave containers. Add weave.role=system label to weave containers. Aug 26, 2015
@squaremo squaremo changed the title Add weave.role=system label to weave containers. Add role label to weave containers so scope can filter Aug 26, 2015
squaremo added a commit that referenced this pull request Aug 27, 2015
Add role label to weave containers so scope can filter
@squaremo squaremo merged commit 8b5992e into master Aug 27, 2015
@squaremo
Copy link
Contributor

Merged on the understanding that this is not really intended to be a feature for end-users. (If it were, I would be concerned about being stuck with the terms "role" and "system".)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants