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

feat(tenant): add label with tenant name for each tenant #910

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

oliverbaehler
Copy link
Collaborator

Initial efforts for the implementation of:
projectcapsule/capsule-proxy#328

Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for capsule-documentation canceled.

Name Link
🔨 Latest commit c44aba2
🔍 Latest deploy log https://app.netlify.com/sites/capsule-documentation/deploys/6566636bd2ea970008683723

Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

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

I dunno why we would need a new recipe, lint when we already have golint.

Comment on lines 18 to 20
tnt.SetLabels(map[string]string{
TenantNameLabel: tnt.Name,
})
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the already existing labels, otherwise, we're gonna overwrite the user ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't agree, using capsule specific labels seems like we are creating just a technical dept for the future. You could say the same thing here, no?
#911

I would prefer using a well-known label here as well.

Copy link
Member

@prometherion prometherion Nov 28, 2023

Choose a reason for hiding this comment

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

I didn't explain properly, here.

Let's say I have a Tenant:

NAME    STATE    NAMESPACE QUOTA   NAMESPACE COUNT   NODE SELECTOR   AGE
solar   Active                     0                                 15s

I'm labelling it for several reasons:

$: kubectl label tnt solar my-label=value
tenant.capsule.clastix.io/solar labeled

$: kubectl get tnt --show-labels
NAME    STATE    NAMESPACE QUOTA   NAMESPACE COUNT   NODE SELECTOR   AGE   LABELS
solar   Active                     0                                 49s   my-label=value

With the tnt.SetLabels provided by your code we're gonna overwrite the user labels, such as my-label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@prometherion Ah right, sorry for the missunderstanding!

Copy link
Collaborator Author

@oliverbaehler oliverbaehler left a comment

Choose a reason for hiding this comment

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

Tried to play around with pre-commit. But it's annoying. Removed the target.

Comment on lines 18 to 20
tnt.SetLabels(map[string]string{
TenantNameLabel: tnt.Name,
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't agree, using capsule specific labels seems like we are creating just a technical dept for the future. You could say the same thing here, no?
#911

I would prefer using a well-known label here as well.

@prometherion
Copy link
Member

Tests are failing but they should have been fixed with the latest commits on main, may I ask you to rebase, please?

@oliverbaehler
Copy link
Collaborator Author

@prometherion I have extend it with a webhook, otherwise there could be potential for escalation between the time of an update and a reconcile. I hope that's alright.

@prometherion prometherion merged commit c58b46c into projectcapsule:main Nov 29, 2023
21 checks passed
@oliverbaehler oliverbaehler added this to the v0.5.0 milestone Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants