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

KUBESAW-12: Convert the health-check goroutine into ToolchainCluster controller #386

Merged
merged 16 commits into from
Apr 22, 2024
Merged

KUBESAW-12: Convert the health-check goroutine into ToolchainCluster controller #386

merged 16 commits into from
Apr 22, 2024

Conversation

fbm3307
Copy link
Contributor

@fbm3307 fbm3307 commented Apr 10, 2024 •
edited
Loading

@codecov Codecov
Copy link

codecov bot commented Apr 10, 2024 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@d0cc930). Click here to learn what that means.
The diff coverage is 63.63%.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master     #386   +/-   ##
=========================================
  Coverage          ?   77.98%           
=========================================
  Files             ?       47           
  Lines             ?     1958           
  Branches          ?        0           
=========================================
  Hits              ?     1527           
  Misses            ?      373           
  Partials          ?       58
Files Coverage Δ
controllers/toolchaincluster/healthchecker.go 93.10% <ø> (ø)
...rs/toolchaincluster/toolchaincluster_controller.go 63.63% <63.63%> (ø)

@fbm3307 fbm3307 marked this pull request as draft April 10, 2024 11:39
This was referenced Apr 10, 2024
Merged
Merged
@fbm3307
Fixing go-lint
84525e0 
Signed-off-by: Feny Mehta <[email protected]>
@fbm3307 fbm3307 marked this pull request as ready for review April 11, 2024 06:43
filariow
filariow requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@filariow filariow left a comment

Choose a reason for hiding this comment

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

nice PR, a few nits and personal preference 😄

controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go Outdated Show resolved Hide resolved
controllers/toolchainclustercache/healthchecker_test.go Show resolved Hide resolved
controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go Outdated Show resolved Hide resolved
controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller_test.go Outdated Show resolved Hide resolved
@fbm3307 fbm3307 mentioned this pull request Apr 17, 2024
Merged
mfrancisc
mfrancisc reviewed Apr 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! Looks good 👍

I have one question for the controller.

I'll take a look at the tests later since I see you are still working on those.

controllers/toolchainclusterhealth/toolchaincluster_healthcheck_controller.go Outdated Show resolved Hide resolved
@fbm3307 fbm3307 mentioned this pull request Apr 17, 2024
Merged
fbm3307 added 3 commits April 17, 2024 15:48
@fbm3307
removing unused clustercacheservice
5556309 
Signed-off-by: Feny Mehta <[email protected]>
@fbm3307
Refactoring the tests
5e77e0c 
Signed-off-by: Feny Mehta <[email protected]>
@fbm3307
Fixing go lint
762118c 
Signed-off-by: Feny Mehta <[email protected]>
@fbm3307 fbm3307 requested review from mfrancisc and filariow April 17, 2024 13:21
fbm3307 added 2 commits April 18, 2024 16:07
@fbm3307
Just convert to controller
3ae2002 
Signed-off-by: Feny Mehta <[email protected]>
@fbm3307
Updated package name
4ec7dc5 
Signed-off-by: Feny Mehta <[email protected]>
MatousJobanek
MatousJobanek reviewed Apr 18, 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.

Good stuff overall 👍
Just a few cosmetic/minor changes

controllers/toolchaincluster/toolchaincluster_controller.go Outdated Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller.go Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller_test.go Outdated Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller_test.go Outdated Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller_test.go Show resolved Hide resolved
controllers/toolchaincluster/healthchecker_test.go
Comment on lines +41 to +46
tests := map[string]struct {
tctype string
apiendpoint string
clusterconditions []toolchainv1alpha1.ToolchainClusterCondition
status toolchainv1alpha1.ToolchainClusterStatus
}{
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this way you improved the unit test 👍 🥇

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

controllers/toolchaincluster/healthchecker_test.go Outdated Show resolved Hide resolved
@fbm3307
Review comments-2
5d6888b 
Signed-off-by: Feny Mehta <[email protected]>
rajivnathan
rajivnathan reviewed Apr 18, 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.

Nice!

mfrancisc
mfrancisc approved these changes Apr 19, 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 🏅

Thanks for the additional tests and improvements !

MatousJobanek
MatousJobanek approved these changes Apr 19, 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.

Good Job 👍

controllers/toolchaincluster/toolchaincluster_controller_test.go Outdated Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller_test.go Outdated Show resolved Hide resolved
controllers/toolchaincluster/toolchaincluster_controller_test.go Outdated Show resolved Hide resolved
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@fbm3307 fbm3307 requested a review from filariow April 22, 2024 06:16
@fbm3307 fbm3307 merged commit e6d41ea into codeready-toolchain:master Apr 22, 2024
8 checks passed
@fbm3307 fbm3307 mentioned this pull request Jun 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajivnathan rajivnathan rajivnathan left review comments

@MatousJobanek MatousJobanek MatousJobanek approved these changes

@filariow filariow filariow 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

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
@fbm3307 @MatousJobanek @mfrancisc @rajivnathan @filariow