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

chore: Add status conditions on nodeclass #6121

Merged
merged 1 commit into from
May 13, 2024
Merged

chore: Add status conditions on nodeclass #6121

merged 1 commit into from
May 13, 2024

Conversation

jigisha620
Copy link
Contributor

Fixes #N/A

Description
This PR adds status conditions on nodeClass for readiness. The final goal is to propagate these status conditions to Nodepool.

How was this change tested?
Tested on my local cluster

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

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

@jigisha620 jigisha620 requested a review from a team as a code owner April 30, 2024 22:01
@netlify Netlify
Copy link

netlify bot commented Apr 30, 2024 •
edited
Loading

✅ Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 0354070
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/6641eec620069000083f046e

@coveralls
Copy link

coveralls commented Apr 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9061530626

Details

  • 37 of 50 (74.0%) changed or added relevant lines in 8 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 82.235%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/nodeclass/status/securitygroup.go 1 2 50.0%
pkg/controllers/nodeclass/status/subnet.go 1 2 50.0%
pkg/controllers/nodeclass/status/ami.go 0 2 0.0%
pkg/controllers/nodeclass/status/nodeclass_readiness.go 15 24 62.5%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/nodeclass/status/controller.go 2 71.7%
Totals Coverage Status
Change from base Build 9037334877: -0.03%
Covered Lines: 5439
Relevant Lines: 6614
💛 - Coveralls

engedaam
engedaam reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@engedaam engedaam left a comment

Choose a reason for hiding this comment

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

Good start! Left a few high level comments

pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/ec2nodeclass_status.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition_test.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/ec2nodeclass_status.go Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition_test.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition_test.go Outdated Show resolved Hide resolved
jonathan-innis
jonathan-innis reviewed May 1, 2024
View reviewed changes
pkg/apis/v1beta1/ec2nodeclass_status.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/ec2nodeclass_status.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/controller.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/status_condition.go Outdated Show resolved Hide resolved
@jigisha620 jigisha620 force-pushed the feat-propagate-sc branch 4 times, most recently from ffa6f5e to be801b2 Compare May 6, 2024 06:28
njtran
njtran reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

Nice work! I have a couple of minor comments.

pkg/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved
pkg/apis/v1beta1/ec2nodeclass_status.go Show resolved Hide resolved
pkg/controllers/nodeclass/status/ami.go Show resolved Hide resolved
pkg/controllers/nodeclass/status/nodeclass_readiness.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/securitygroup.go Show resolved Hide resolved
pkg/controllers/nodeclass/status/nodeclass_readiness.go Outdated Show resolved Hide resolved
pkg/controllers/nodeclass/status/subnet.go Show resolved Hide resolved
test/pkg/environment/common/expectations.go Outdated Show resolved Hide resolved
website/content/en/docs/concepts/nodeclasses.md Outdated Show resolved Hide resolved
njtran
njtran approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

LGTM nice work!

@jigisha620 jigisha620 merged commit 9751fd0 into aws:main May 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-innis jonathan-innis jonathan-innis left review comments

@engedaam engedaam engedaam left review comments

@njtran njtran njtran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jigisha620 @coveralls @njtran @jonathan-innis @engedaam