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

Cluster refactor: Split out SysInstaller, simplify check-fixes #721

Merged
merged 10 commits into from
Jan 27, 2021

Conversation

nicholastmosher
Copy link
Contributor

@nicholastmosher nicholastmosher commented Jan 27, 2021

This is a good start to some refactoring that's needed in the cluster crate. In this PR I've extracted the sys chart installation logic into its own SysInstaller type, which means we have less code duplication and cross-dependency between the Local and K8 installers.

Another change here is that I've added an attempt_fix optional function to the ClusterCheck trait, which allows instances of that trait to define the means by which it can attempt to fix a failed check. Previously, this fixing logic was very far removed from the check itself, so this consolidates things nicely. It also has the effect of unblocking the "incremental progress checks" for the k8 installer. Now, using the k8 installer fluvio cluster start will print checking outputs as they are run, the same way that fluvio cluster start --local already does today.

This checks a number of the boxes from #705

@nicholastmosher nicholastmosher linked an issue Jan 27, 2021 that may be closed by this pull request
5 tasks
@nicholastmosher nicholastmosher requested a review from sehz January 27, 2021 16:40
@nicholastmosher nicholastmosher removed a link to an issue Jan 27, 2021
5 tasks
Copy link
Contributor

@sehz sehz 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 with few minor points.

src/cluster/src/check/mod.rs Outdated Show resolved Hide resolved
src/cluster/src/check/mod.rs Outdated Show resolved Hide resolved
src/cluster/src/check/mod.rs Show resolved Hide resolved
src/cluster/src/check/mod.rs Show resolved Hide resolved
src/cluster/src/cli/start/sys.rs Show resolved Hide resolved
src/cluster/src/cli/start/sys.rs Outdated Show resolved Hide resolved
src/cluster/src/cli/start/sys.rs Outdated Show resolved Hide resolved
src/cluster/src/sys.rs Show resolved Hide resolved
@nicholastmosher
Copy link
Contributor Author

It'd be nice to merge and publish infinyon/fluvio-helm#11 before this, so that we can bump our dependency to fluvio-helm:0.4.1 and use the latest version. The big benefit there is that the new fluvio-helm updates do not forward all of the noise from the helm command

@nicholastmosher nicholastmosher requested a review from sehz January 27, 2021 20:42
Copy link
Contributor

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

@sehz sehz merged commit 540dd59 into infinyon:master Jan 27, 2021
@nicholastmosher nicholastmosher deleted the cluster-refactor branch January 27, 2021 21:20
sehz pushed a commit to sehz/fluvio that referenced this pull request Feb 2, 2021
…yon#721)

* Consolidate start and upgrade checks

* Factor out SysInstaller from k8 and local installers

* Fix broken intra-doc links

* Add 'attempt_fix' to ClusterCheck

* Add progress rendering checks to K8Installer

* Move check implementations into ClusterCheck impls

* Apply cargo fmt

* Apply cargo clippy

* Add builder method for conditional chaining

* Bump fluvio-helm dependency to 0.4.1
simlay pushed a commit that referenced this pull request Feb 25, 2021
* Consolidate start and upgrade checks

* Factor out SysInstaller from k8 and local installers

* Fix broken intra-doc links

* Add 'attempt_fix' to ClusterCheck

* Add progress rendering checks to K8Installer

* Move check implementations into ClusterCheck impls

* Apply cargo fmt

* Apply cargo clippy

* Add builder method for conditional chaining

* Bump fluvio-helm dependency to 0.4.1
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.

2 participants