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

Create NS Annotation label Made a Constant #1533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Create NS Annotation label Made a Constant #1533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/kudoctl/util/kudo/kudo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ func (c *Client) CreateNamespace(namespace, manifest string) error {
}
ns.TypeMeta.Kind = "Namespace"
ns.Name = namespace
ns.Annotations["created-by"] = "kudo-cli"
ns.Annotations[label.CreatedByAnnotation] = "kudo-cli"

_, err := c.kubeClientset.CoreV1().Namespaces().Create(ns)
return err
Expand Down
2 changes: 2 additions & 0 deletions pkg/util/kudo/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,6 @@ const (

// Last applied state for three way merges
LastAppliedConfigAnnotation = "kudo.dev/last-applied-configuration"

CreatedByAnnotation = "kudo.dev/created-by"
Copy link
Contributor

Choose a reason for hiding this comment

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

The created-by semantics is somewhat strange: CLI also creates Instance, Operator, and OperatorVersion resources - should we now mark them too? Instance controller creates all other resources - should we then add kudo.dev/created-by: instance-controller?
Our labels (seen above) have been about what-this-is and not who-created-it. So maybe:

Suggested change
CreatedByAnnotation = "kudo.dev/created-by"
InstanceNamespaceAnnotation = "kudo.dev/instance-namespace"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we know that the namespace is for an instance... currently it is. The ns could be for an operator needs. I am simply looking for a convenient way to identify that this is a kudo managed (or created thing). This seems like the function of annotations.

I am for annotating a created-by for each of the resources you mention Instance, Operator, etc. However in these cases, there is less confusion over who created them... or more importantly who manages them. One could claim that an Instance can be created externally to kudo... with enough knowledge this would be true... but there wouldn't be any argument over who manages the Instance... it is created with the intent to have it managed by the kudo manager.

As we start to take on the creation of resources not owned by kudo. Cluster-wide resources, namespace, serviceaccount, crd, etc. It makes sense IMO to have a record as an annotation of "who" is managing or in joint management of these resources.

there could additionally be benefit in similar labels which can be used as selection... but that is a separate concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

However in these cases, there is less confusion over who created them... or more importantly who manages them

Fair point though I disagree about "managing" part as things are generally created by the CLI and managed by the manager. In this case, we should stay consistent and also annotate I/O/OV resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've thought about it, and I like this annotation. Should we ever introduce kudo uninstall it might become very useful. However, let's do it consistently: we should annotate all resources created by the CLI with it:

  • all package resources such as Instance, Operator and OperatorVersion
  • all kudo init resources
    /cc @kensipe

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree... but it shouldn't need to be all in one PR right? I'll create an issue to track.
I completely agree that kudo should have an annotation on all created resources

Copy link
Contributor

@zen-dog zen-dog Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason to split this functionality over multiple PRs? I imagine the whole thing being ~30 SLOCs

Copy link
Contributor

Choose a reason for hiding this comment

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

Just came by this PR. Why did we introduce new annotation, just wondering? Can't we use something that we already. use for all the other objects in enhancer? Like e.g. kudo.HeritageLabel ? 🤔 sorry if you already had this discussion in the past :)

)