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

Move all healthcheck-related code to pkg/healthcheck #1492

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

klingerf
Copy link
Contributor

This branch is a substantial refactor to the way our health check code was setup:

  • All checks are now defined in pkg/healthcheck/healthcheck.go
  • Check definitions use the healthcheck.checkers struct instead of protobuf
  • Checks can now be marked as fatal, which will skip all remaining checks on failure
  • Checks are now binary (passing/failing); we no longer distinguish between failing and errored
  • The MockKubeApi struct has been removed, since it was no longer used
  • The KubernetesApi interface has been removed, since it only had one implementation after removing the mock

This refactor is in support of #1470, #1471, #1473, #1477, and #1478.

Signed-off-by: Kevin Lingerfelt <[email protected]>
Copy link
Contributor

@dadjeibaah dadjeibaah left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 . I like that we no longer run the remaining checks if one of the checks fail. Do you think there is value in displaying the checks that did not run so that the user is aware of them just in case they need to debug? Is something like this already going to be addressed in the --pre and --post flight checks?

if err != nil {
os.Exit(2)
}
configureAndRunChecks(options)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't part of the change, but the use of the possessive pronoun "your" is a little jarring especially when compared to other command descriptions. We may want to revisit this in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh, yeah, this is great feedback. I partially rewrote the description for this command in #1495, and I'll address this suggestion there.

As far as printing the skipped check goes, I'm a bit on the fence. If the user fails a fatal check, then they must fix it before being able to successfully use the CLI. In that case, printing the checks that were skipped might distract from the failure that requires fixing.

Copy link

@rmars rmars left a comment

Choose a reason for hiding this comment

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

🌟 🔧 🔥 whoa! this really simplifies testing too! nice refactor!

linkerd-api[kubernetes]: control plane can talk to Kubernetes..............[ok]
linkerd-api[prometheus]: control plane can talk to Prometheus..............[ok]
linkerd-version: can get the latest version................................[ok]
Copy link

Choose a reason for hiding this comment

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

slight naming nit: can we use "can query for the latest version" or "can determine the latest version" instead? "can get" to me implies some downloading (I wasn't sure what it meant at first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yep, great call! I like "can determine the latest version" a lot -- will update.

Signed-off-by: Kevin Lingerfelt <[email protected]>
@klingerf klingerf merged commit e97be1f into master Aug 20, 2018
@klingerf klingerf deleted the kl/check-refactor branch August 20, 2018 23:51
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