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

Add config field to the ControllerOption #3615

Merged

Conversation

liuyuanchun11
Copy link
Contributor

Add config field to the ControllerOption, pass the config parameter to the controllers, and the controllers can use config to create a custom clientset

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 20, 2024
@@ -37,6 +38,8 @@ type ControllerOption struct {
InheritOwnerAnnotations bool
WorkerThreadsForPG uint32
WorkerThreadsForGC uint32

Config *rest.Config
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have any plans or what the custom client needs?

Copy link
Member

Choose a reason for hiding this comment

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

I think we add some comments to describe why is this needed and what does it aim to do is better: ) @liuyuanchun11

Copy link
Contributor Author

@liuyuanchun11 liuyuanchun11 Jul 20, 2024

Choose a reason for hiding this comment

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

Sorry for not explaining clearly. I am maintaining a project based on the volcano community version, which contains some custom resource controllers and plug-ins, etc. Now I need to add a custom resource clientset, similar to VolcanoClient. The clientset of this custom resource is obviously not suitable to be placed in the ControllerOption structure, so I want to put rest.Config in the ControllerOption, so that it can be passed to my new controller, and then call NewForConfigOrDie(config) to generate clientset

Copy link
Member

Choose a reason for hiding this comment

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

Will the maintained features be fed back to the community? IMO, there is no need to introduce unused variables.
Just my opinion.If it is an overall feature, it feels more reasonable to introduce this variable

@@ -37,6 +38,9 @@ type ControllerOption struct {
InheritOwnerAnnotations bool
WorkerThreadsForPG uint32
WorkerThreadsForGC uint32

// Config holds the common attributes that can be passed to a Kubernetes client on initialization.
Copy link
Member

@Monokaix Monokaix Jul 24, 2024

Choose a reason for hiding this comment

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

Kubernetes client on initialization. =》Kubernetes client and controllers registered by the users can use it.

@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2024
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2024
@liuyuanchun11 liuyuanchun11 force-pushed the controller_addconfig branch 3 times, most recently from 95dd451 to 9addcf9 Compare July 24, 2024 23:57
@william-wang
Copy link
Member

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024
@wangyang0616
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2024
@volcano-sh-bot volcano-sh-bot merged commit 5633ec4 into volcano-sh:master Jul 27, 2024
14 checks passed
@liuyuanchun11 liuyuanchun11 deleted the controller_addconfig branch July 29, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants