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

chore: rewrite machine controller names #351

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

guikcd
Copy link
Contributor

@guikcd guikcd commented May 25, 2023

Fix: #3741
EDIT: only machine related controller names changed.

Description

Rename controller names to respect the codebase structure.
I've not changed pkg/controllers/metrics/state/controller.go which has name metric_scraper, please confirm me that this specific name can be changed to metrics.state and have no impact on the rest of the Karpenter project.

How was this change tested?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@guikcd guikcd requested a review from a team as a code owner May 25, 2023 16:40
@guikcd guikcd requested a review from engedaam May 25, 2023 16:40
@jackfrancis
Copy link
Contributor

This might be a great change, but I'm not sure it would classify as a fix? Seems like something that may have breaking side-effects and so we'd want to ensure it's only rolled out in a minor release and called out in release notes.

@guikcd guikcd changed the title fix: rewrite controller names chore: rewrite controller names May 25, 2023
@guikcd
Copy link
Contributor Author

guikcd commented May 25, 2023

This might be a great change, but I'm not sure it would classify as a fix? Seems like something that may have breaking side-effects and so we'd want to ensure it's only rolled out in a minor release and called out in release notes.

You're right, a chore is probably more appropriate, ✔️

@coveralls
Copy link

coveralls commented May 25, 2023

Pull Request Test Coverage Report for Build 5091393148

  • 2 of 3 (66.67%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 81.443%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/machine/garbagecollection/controller.go 0 1 0.0%
Totals Coverage Status
Change from base Build 5074239499: 0.1%
Covered Lines: 6921
Relevant Lines: 8498

💛 - Coveralls

@jonathan-innis
Copy link
Member

have no impact on the rest of the Karpenter project

This has impact on metrics controller name labels so it technically classifies as breaking, but I think that this change makes sense.

@jonathan-innis
Copy link
Member

Looking over the whole codebase, it probably makes sense to scope like this based on the directory structure:

  • consistency
  • counter
  • deprovisioning
  • machine.garbagecollection
  • machine.lifecycle
  • machine.termination
  • metrics.pod
  • metrics.provisioner
  • metrics.state
  • node
  • provisioning
  • provisioning.trigger
  • state.daemonset
  • state.machine
  • state.node
  • state.pod
  • state.provisioner

jonathan-innis
jonathan-innis previously approved these changes May 26, 2023
Copy link
Member

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Made some comments on additional changes that you can do to make controller names consistent throughout!

@guikcd
Copy link
Contributor Author

guikcd commented May 26, 2023

LGTM 🚀 Made some comments on additional changes that you can do to make controller names consistent throughout!

Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

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

The machine changes look ok since they haven't been released, but I don't think we can change the others without some significant discussion and possibly linking it to v1beta1. It would silently break anyone that is using these for alarming.

@guikcd
Copy link
Contributor Author

guikcd commented May 26, 2023

The machine changes look ok since they haven't been released, but I don't think we can change the others without some significant discussion and possibly linking it to v1beta1. It would silently break anyone that is using these for alarming.

Make totally sense. PR updated to only modify machine controller names ✅

@guikcd guikcd changed the title chore: rewrite controller names chore: rewrite machine controller names May 26, 2023
@jonathan-innis
Copy link
Member

It would silently break anyone that is using these for alarmin

Makes sense @tzneal. We can rope this into the metrics/observability discussion.

@jonathan-innis
Copy link
Member

@guikcd Would you mind making the same changes to machine controllers in the aws/karpenter repo?

Copy link
Member

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@guikcd
Copy link
Contributor Author

guikcd commented May 26, 2023

@guikcd Would you mind making the same changes to machine controllers in the aws/karpenter repo?

aws/karpenter-provider-aws#3945

Copy link
Contributor

@tzneal tzneal left a comment

Choose a reason for hiding this comment

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

lgtm

@jonathan-innis jonathan-innis merged commit 1e56dd5 into kubernetes-sigs:main Jun 1, 2023
@guikcd guikcd deleted the odd_casing branch June 1, 2023 21:51
This pull request was closed.
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.

Odd casing on controller names
5 participants