Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

cli/cmd: cleanups part 2 #1015

Merged
merged 15 commits into from
Oct 16, 2020
Merged

cli/cmd: cleanups part 2 #1015

merged 15 commits into from
Oct 16, 2020

Conversation

invidian
Copy link
Member

@invidian invidian commented Sep 25, 2020 •
edited
Loading

Includes commits from #1013

Part of #603.

@invidian invidian force-pushed the invidian/cli-cleanup-part2 branch 2 times, most recently from 6efb011 to 9e346e0 Compare September 25, 2020 12:20
@invidian invidian mentioned this pull request Sep 25, 2020
Merged
@invidian invidian force-pushed the invidian/cli-cleanup-part2 branch 2 times, most recently from c02067c to 1e1b630 Compare October 8, 2020 09:53
@invidian invidian requested review from surajssd, iaguis and knrt10 October 8, 2020 10:26
knrt10
knrt10 reviewed Oct 14, 2020
View reviewed changes
Copy link
Member

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Nice work

@invidian invidian force-pushed the invidian/cli-cleanup-part2 branch from 1e1b630 to 5c70ab8 Compare October 14, 2020 06:42
knrt10
knrt10 previously approved these changes Oct 14, 2020
View reviewed changes
surajssd
surajssd suggested changes Oct 14, 2020
View reviewed changes
cli/cmd/component-delete.go
@@ -23,6 +23,7 @@ import (

"github.com/kinvolk/lokomotive/pkg/components"
"github.com/kinvolk/lokomotive/pkg/components/util"
"github.com/kinvolk/lokomotive/pkg/config"
Copy link
Member

Choose a reason for hiding this comment

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

I think all the component delete code can be coalesced into one commit. It is very confusing to read some change here and some change there. I think it makes sense to add them all to one commit and then list all the changes that have happened.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want to review changes aggregated, you can use git diff with range or "Changed files" tab in GitHub UI. I wouldn't lose the information of those changes, as each of them is a small independent step making things a bit better.

Also, having more commits makes bisecting easier in case of bugs, which definitely might be introduced by those changes.

@invidian invidian force-pushed the invidian/cli-cleanup-part2 branch from 5c70ab8 to e2efa62 Compare October 15, 2020 09:07
@invidian invidian requested a review from surajssd October 15, 2020 09:08
surajssd
surajssd reviewed Oct 16, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

Code is ok, please change the name of func to make it more understandable what it is doing.

invidian added 12 commits October 16, 2020 15:22
@invidian
cli/cmd: make initialize() return error instead of using Fatalf
b5a599b 
To remove a dependency on log.Entry to make moving this code around
easier and to avoid hiding function complexity from logger.

Part of #630

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make initializeTerraform() return error
ac1dfa0 
To remove a dependency on log.Entry to make moving this code around
easier and to avoid hiding function complexity from logger.

Also, it no longer requires logger, so argument has been removed.

Part of #630

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make clusterExists return error
8fc5016 
To remove a dependency on log.Entry to make moving this code around
easier and to avoid hiding function complexity from logger.

Also, it no longer requires logger, so argument has been removed.

Part of #630

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make upgradeComponent() return error
cdf5f77 
To remove a dependency on log.Entry to make moving this code around
easier and to avoid hiding function complexity from logger.

Part of #630

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make component apply use errors instead of Fatalf
e707440 
To soften the dependency on log.Entry and to separate it strictly
from *cobra.Command, so it is easier to move this code around in the
future.

Part of #630

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make applyComponents() annotate all errors
b83dfca 
So they are easier to trace if something fails, which makes debugging
easier.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: ensure error from deleteComponents() is annotated
7bfe85d 
To make tracing and debugging easier.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: simplify selecting components to delete
297fb69 
So it can be moved to function which can be shared with component
apply code.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: improve readability of runDelete()
d6ffddc 
By moving confirmation message declaration from function call into a
variable.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: break down component delete into smaller functions
10032d0 
So code can be re-used in component apply function.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make componentApply() use componentNames() function
c34cc97 
To de-duplicate logic of selecting components to operate on.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: remove use of ... from deleteComponents()
b1af591 
As it does not add any value, as we create a slice of objects anyway
instead of specifying them by hand.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: remove use of ... from applyComponents()
7124684 
As it does not add any value, as we create a slice of objects anyway
instead of specifying them by hand.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: make applyComponents() accept slice of objects
45dc74c 
Instead of slice of names. This shifts the error handling of getting
components to earlier stage, so we don't apply one component and then
fail, because other one does not exist.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian
cli/cmd: add comment to applyComponents()
d62bba8 
To make distinction between this function and componentApply() more
clear.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@invidian invidian force-pushed the invidian/cli-cleanup-part2 branch from e2efa62 to d62bba8 Compare October 16, 2020 14:14
@invidian invidian requested a review from surajssd October 16, 2020 14:15
surajssd
surajssd approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

LGTM

@invidian invidian merged commit 48cb56d into master Oct 16, 2020
@invidian invidian deleted the invidian/cli-cleanup-part2 branch October 16, 2020 15:00
@invidian invidian mentioned this pull request Nov 2, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@surajssd surajssd surajssd approved these changes

@knrt10 knrt10 knrt10 left review comments

@iaguis iaguis Awaiting requested review from iaguis

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

Successfully merging this pull request may close these issues.
3 participants
@invidian @surajssd @knrt10