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

KUBESAW-187: Adjust ksctl adm restart command to use rollout-restart #79

Merged
merged 52 commits into from
Nov 27, 2024

Conversation

fbm3307
Copy link
Contributor

@fbm3307 fbm3307 commented Sep 12, 2024

This is to adjust. the restart command logic
it does the following

  1. If the command is run for host operator, it restart the whole host operator.(it deletes olm based pods(host-operator pods),
    waits for the new deployment to come up, then uses rollout-restart command for non-olm based - registration-service)
  2. If the command is run for member operator, it restart the whole member operator.(it deletes olm based pods(member-operator pods),
    waits for the new deployment to come up, then uses rollout-restart command for non-olm based deployments - webhooks)

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.

Could you please add a few print info lines for better UX?

pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
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 left few minor comments. Also haven't got trough the test code since it looks still WIP.

pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
@fbm3307 fbm3307 marked this pull request as ready for review September 25, 2024 09:09
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
pkg/cmd/adm/unregister_member_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
go.mod Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart.go Show resolved Hide resolved
pkg/cmd/adm/unregister_member_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/unregister_member_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
pkg/cmd/adm/restart_test.go Outdated Show resolved Hide resolved
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
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.

Looks good, thanks for addressing all my comments 👍

@fbm3307 fbm3307 merged commit a89cb52 into kubesaw:master Nov 27, 2024
6 of 8 checks passed
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 65.62500% with 44 lines in your changes missing coverage. Please review.

Project coverage is 67.78%. Comparing base (5a14398) to head (70c53e7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/cmd/adm/restart.go 65.60% 33 Missing and 10 partials ⚠️
pkg/cmd/adm/unregister_member.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
- Coverage   68.00%   67.78%   -0.22%     
==========================================
  Files          44       44              
  Lines        3175     3216      +41     
==========================================
+ Hits         2159     2180      +21     
- Misses        825      841      +16     
- Partials      191      195       +4     
Files with missing lines Coverage Δ
pkg/cmd/adm/unregister_member.go 54.54% <66.66%> (ø)
pkg/cmd/adm/restart.go 57.14% <65.60%> (-2.15%) ⬇️

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.

4 participants