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

Jump to bottom

Unify workload[s] names in the tmc controller.go #2936

Closed
davidfestal opened this issue Mar 30, 2023 · 5 comments · Fixed by #2941
Closed

Unify workload[s] names in the tmc controller.go #2936

davidfestal opened this issue Mar 30, 2023 · 5 comments · Fixed by #2941
Assignees
@dhanusaputra
Labels
area/transparent-multi-cluster Related to scheduling of workloads into pclusters. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.

Comments

@davidfestal
Copy link
Member

davidfestal commented Mar 30, 2023 •
edited
Loading

In the /tmc/pkg/server/controllers.go, some workload related controllers are installed with installWorkload... and some with installWorkloads.....
The relates to the fact that the import alias sometimes contains the s, sometimes not.
It make sense to unify the names, as well as the names of the related methods.

This is a followup of this PR comment.
@ncdc ncdc added this to kcp Mar 30, 2023
@davidfestal davidfestal added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Mar 30, 2023
@github-project-automation github-project-automation bot moved this to New in kcp Mar 30, 2023
@davidfestal davidfestal added the area/transparent-multi-cluster Related to scheduling of workloads into pclusters. label Mar 30, 2023
@davidfestal
Copy link
Member Author

cc @p0lyn0mial

@davidfestal davidfestal mentioned this issue Mar 30, 2023
Merged
@dhanusaputra
Copy link
Contributor

hi, can i help with this please, thanks

@p0lyn0mial
Copy link
Contributor

hi, can i help with this please, thanks

absolutely! please, feel free to self-assign the issue, thanks!

@davidfestal
Copy link
Member Author

hi, can i help with this please, thanks

absolutely! please, feel free to self-assign the issue, thanks!

Yes, Thanks !

As for the change to be done, I would vote for keeping the workload without an s, since it is the package name.

What do other think ?

Let's vote:

  • add a 👍 to this comment to use workload without an s everywhere
  • add a 👎 to this comment to use workloads everywhere

@dhanusaputra
Copy link
Contributor

/assign

@dhanusaputra dhanusaputra mentioned this issue Apr 4, 2023
Merged
@github-project-automation github-project-automation bot moved this from New to Done in kcp Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dhanusaputra dhanusaputra

Labels
area/transparent-multi-cluster Related to scheduling of workloads into pclusters. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.
Projects
kcp
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🌱 Unify workload[s] names in the tmc controller.go dhanusaputra/kcp
3 participants
@davidfestal @p0lyn0mial @dhanusaputra