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

SpaceProvisionerConfig should only be ready when underlying ToolchainCluster is ready #1033

Merged
merged 11 commits into from
Jun 13, 2024
Merged

SpaceProvisionerConfig should only be ready when underlying ToolchainCluster is ready #1033

merged 11 commits into from
Jun 13, 2024

Conversation

metlos
Copy link
Contributor

@metlos metlos commented May 16, 2024 •
edited

Fix the SpaceProvisionerConfig controller to only set the SPC ready when the referenced ToolchainCluster is ready. This should make sure we never provision to offline clusters.

Associated PRs:

@metlos
Fix the SpaceProvisionerConfig controller to only set the SPC ready
a00cff5 
when the referenced ToolchainCluster is ready. This should make sure we
never provision to offline clusters.
@metlos metlos mentioned this pull request May 16, 2024
Merged
@metlos metlos mentioned this pull request May 16, 2024
Merged
mfrancisc
mfrancisc reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Nice Job 👍

I have only few questions related to the unit tests.

controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller_test.go Show resolved Hide resolved
controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller_test.go Outdated Show resolved Hide resolved
controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller_test.go Show resolved Hide resolved
mfrancisc
mfrancisc approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Thanks for addressing my comments.

rajivnathan
rajivnathan approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@rajivnathan rajivnathan left a comment

Choose a reason for hiding this comment

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

Looks great overall 👍 , just made a small suggestion

controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller.go Outdated Show resolved Hide resolved
@metlos
Copy link
Contributor Author

metlos commented May 29, 2024

/retest

MatousJobanek
MatousJobanek approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

nice 👍 🚀
we can add more validations long-term - eg. checking the threshold against the actual usage, so some of the capacity-management logic would be moved here. In this way, it would be visible directly in the status of the SPC if the cluster is available for provisioning or not and for what reason.

controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller.go Show resolved Hide resolved
controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller.go Outdated Show resolved Hide resolved
MatousJobanek
MatousJobanek approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

cool, thanks for the additional changes 👍

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, metlos, mfrancisc, rajivnathan

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:
  • OWNERS [MatousJobanek,mfrancisc,rajivnathan]

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

@metlos
Copy link
Contributor Author

metlos commented Jun 7, 2024

/retest

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
13.0% Duplication on New Code

See analysis details on SonarCloud

@codecov Codecov
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (05e1de3) to head (5aac024).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
+ Coverage   84.72%   84.81%   +0.08%     
==========================================
  Files          55       55              
  Lines        4909     4918       +9     
==========================================
+ Hits         4159     4171      +12     
+ Misses        577      573       -4     
- Partials      173      174       +1
Files Coverage Δ
...isionerconfig/spaceprovisionerconfig_controller.go 79.66% <83.33%> (+9.66%) ⬆️

... and 2 files with indirect coverage changes

@MatousJobanek MatousJobanek merged commit 93dda62 into codeready-toolchain:master Jun 13, 2024
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajivnathan rajivnathan rajivnathan approved these changes

@MatousJobanek MatousJobanek MatousJobanek approved these changes

@mfrancisc mfrancisc mfrancisc approved these changes

@xcoulon xcoulon Awaiting requested review from xcoulon xcoulon is a code owner

@alexeykazakov alexeykazakov Awaiting requested review from alexeykazakov alexeykazakov is a code owner

@ranakan19 ranakan19 Awaiting requested review from ranakan19 ranakan19 is a code owner

@sbryzak sbryzak Awaiting requested review from sbryzak sbryzak is a code owner

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@metlos @MatousJobanek @mfrancisc @rajivnathan @openshift-merge-robot