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

exposed service account name for tcp pods #441

Merged

Conversation

hamza-boudouche
Copy link
Contributor

434

Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for kamaji-documentation ready!

Name Link
🔨 Latest commit b70c0c6
🔍 Latest deploy log https://app.netlify.com/sites/kamaji-documentation/deploys/66276587c9028d0008e792a1
😎 Deploy Preview https://deploy-preview-441--kamaji-documentation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@prometherion prometherion added this to the v0.6.0 milestone Apr 19, 2024
api/v1alpha1/tenantcontrolplane_types.go Outdated Show resolved Hide resolved
cmd/manager/cmd.go Outdated Show resolved Hide resolved
handlers.TenantControlPlaneDefaults{DefaultDatastore: datastore},
handlers.TenantControlPlaneDefaults{
DefaultDatastore: datastore,
DefaultServiceAccount: serviceAccount,
Copy link
Member

Choose a reason for hiding this comment

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

No need for this, it can be achieved with the kubebuilder marker, see previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I made a follow up comment about this and you didn't comment on it afterwards so I supposed you were ok with it, but apparently not.
The reason behind this was to be able to use a specific service account to be considered as "default" by Kamaji, and that can be different from the actual default service account. This can be handy when we use the management cluster for something other than just running Kamaji at the same time.
This will have no effects on the users that do not need this since it will be set to "default" by default.

Copy link
Member

Choose a reason for hiding this comment

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

I get your point, and sorry if I missed the discussion: too many things happening at the same time.

I would avoid this kind of flag, and rely on configuration over convention: it means that if you need to configure a specific ServiceAccount other tools can be used to enforce this behaviour, such as Kyverno policies, or ClusterClass in terms of Cluster API.

controllers/tenantcontrolplane_controller.go Outdated Show resolved Hide resolved
internal/webhook/handlers/tcp_defaults.go Outdated Show resolved Hide resolved
@hamza-boudouche hamza-boudouche marked this pull request as draft April 19, 2024 08:53
@hamza-boudouche hamza-boudouche marked this pull request as ready for review April 19, 2024 12:28
@prometherion prometherion added the enhancement New feature or request label Apr 22, 2024
@prometherion
Copy link
Member

@hamza-boudouche sorry, some conflicts must be resolved since another PR of yours has been merged.

Once solved, getting ready to get it merged!

@prometherion prometherion merged commit 3761686 into clastix:master Apr 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants