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.

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

Possibility to specify namespace parameter makes configuration confusing #35

Closed
ccremer opened this issue Jul 6, 2020 · 11 comments
Closed

Comments

@ccremer
Copy link
Contributor

ccremer commented Jul 6, 2020

Let's talk about the namespace that human users can specify for Seiso to operate/cleanup stuff in.

For history and orphans command this is the help page:

seiso images history [NAMESPACE/IMAGE] [flags]
...
Global Flags:
-n, --namespace string   Cluster namespace of current context

So, which one should take precedence when given seiso -n flag arg/image?

The initial reason for the [NAMESPACE/IMAGE] CLI syntax was that any image registry could be specified for cleanup. OpenShift is but the first supported registry, with Docker Hub registry to follow. However, the current implementation, which takes into account of currently configured kubeconfig file, but also the --namespace flag, makes it confusing as to which value should now be specified. It's also possible to do seiso images history -n flag imagename and it would cleanup the flag/imagename image repository. Even worse, with local kubeconfig and seiso images history imagename it also transforms to kube/imagename if the kubeconfig namespace is kube.

While at first a great idea, this will not be possible for non-kubeconfig-based registries, such as Docker Hub or Quay.io. Granted, we have implemented this (yet?), but I'm confused in the CLI usage.

Let's discuss and clarify how we want to make Seiso more streamlined

@zugao
Copy link
Contributor

zugao commented Jul 6, 2020

If I remember it correctly the idea was to move from seiso images history [NAMESPACE/IMAGE] [flags] to -n, --namespace string flag. So over time seiso images history [NAMESPACE/IMAGE] [flags] would be deprecated. In case the default namespace is not supported we should simply raise an exception.

@ccremer
Copy link
Contributor Author

ccremer commented Jul 8, 2020

So over time seiso images history [NAMESPACE/IMAGE] [flags] would be deprecated.

Do you have a written reference to that? I don't recall this decision at all...
In the kubernetes ecosystem, every image repository has this registry/org/imagename:tag notation, why should it be different in Seiso? I remember that Seiso was supposed to support Docker and GitLab registries too (especially GitLab!) Even the current README suggest that future registries may be supported. The current codebase even has code examples in it where the Docker client is being used, suggesting more supported registries in the future. Our customers use GitLab registries already today and are in need of a cleanup tool like Seiso.

I highly doubt we would want to specify the image registries like seiso -r gitlab.com -g appuio -n example-django images history app is a convenient notation when we want to cleanup images pushed to the GitLab registry. Every other Kubernetes-related tool would support the / based notation, like seiso images history gitlab.com/appuio/example-django/app. The -n would be useless, as many image registries do not need the kube client to work with. The oc tag command also expects the / based notation with the namespace given there.

I would actually ignore the --namespace flag when the images commands are being used, and make the / based notation mandatory. For cleaning up configmaps and Secrets, the story looks different. And they can, since they do another thing entirely.

@zugao
Copy link
Contributor

zugao commented Jul 9, 2020

There's no written reference to that, I discussed the -n flag with Peter during some refactoring in a video call. I'm not sure what exactly was the reasoning behind this decision as it was @bittner idea to introduce that flag (to make Seiso more flexible?). Apparently it's not the best solution as you pointed out in your comment and it make the whole thing confusing. We could have a call together and see how we should proceed forward.

@bittner
Copy link
Contributor

bittner commented Jul 13, 2020

It should just work the same way as oc does. Simple as that. There should be no need to reinvent the wheel, no need for a developer used to the OpenShift CLI to learn a new behavior.

I believe, with oc the namespace is like the place where you start your command (similar to pwd in a shell). When you specify the path explicitly this takes precedence. I think that's oc's behavior. Please, let's verify this and keep the behavior in sync between the two CLI's.

@ccremer
Copy link
Contributor Author

ccremer commented Jul 14, 2020

Just because oc does it like that it doesn't mean that it's a good behaviour.

Please, let's verify this

That just proved that the behaviour is confusing for you as well, since you would need to verify this to know for sure 😄

I dislike keeping the --namespace flag for those 2 commands but if you want it desperately go ahead... However I would definitely NOT deprecate the registry/org/imagename:tag arg though and let this even take precedence.

@bittner
Copy link
Contributor

bittner commented Jul 14, 2020

I agree: I would not deprecate or remove the possibility to explicitly specify the full path for an image.

Let me reaffirm, I believe it's a good idea to keep the behavior in sync with the tools the Kubernetes ecosystem provides.

@ccremer
Copy link
Contributor Author

ccremer commented Jul 14, 2020

To be clear: Kubernetes/kubectl doesn't have ImageStream or tagging capabilities, that's just OpenShift-specific.
But fine for me.
@zugao your thoughts?

@zugao
Copy link
Contributor

zugao commented Jul 15, 2020

I would try to separate them. What I mean by this:

  • for images ignore or raise an error whenever --namespace flag is used
  • for configmaps and secrets use exclusively --namespace flag

This solution is less confusion for users and for developers, no need to worry about precedence.

@ccremer
Copy link
Contributor Author

ccremer commented Jul 15, 2020

Hm, for ConfigMaps and Secrets it makes more sense to use the context and namespace from kubeconfig/kubectl first, with the possibility to override with --namespace. That would be in sync with kubectl usage.

@bittner
Copy link
Contributor

bittner commented Jul 16, 2020

Can you re-evaluate the current behavior? I believe it is already in sync with the oc CLI. There should be no need for a change.

@ccremer
Copy link
Contributor Author

ccremer commented Aug 10, 2020

As implemented in #34 , the current order of precedence is as follows:

ARG > --namespace flag > namespace from kubeconfig > error

However, the --namespace and kubeconfig options only apply when using Openshift. For Docker/GitLab registries, this might not be possible later, even though it's not implemented yet.

@ccremer ccremer closed this as completed Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants