feat: initial implementation of coder-doctor by jawnsy · Pull Request #3 · coder/coder-doctor

jawnsy · 2021-08-07T16:48:50Z

No description provided.

shortcut-integration · 2021-08-07T16:48:52Z

This pull request has been linked to Clubhouse Story #15961: Initial implementation of coder-doctor framework.

internal/cmd/root.go

kylecarbs · 2021-08-11T04:08:40Z

Such a massive fan of this! We can use this package to validate clusters are properly configured when being added too. 🥳🥳🥳

internal/checks/kubernetes/version.go

deansheather · 2021-08-11T12:17:50Z

internal/checks/kubernetes/version_test.go

+		},
+	}
+
+	for _, test := range tests {


you should autogenerate some tests (with dummy data for version fields we don't care about) for all of our supported versions

internal/checks/kubernetes/version_test.go

internal/api/writer.go

internal/api/types.go

kylecarbs · 2021-08-14T04:07:16Z

internal/api/types.go

+
+var _ = fmt.Stringer(StatePassed)
+
+type CheckState int


Is there a reason we make these integers rather than strings?

That way we could eliminate the string function, and set the values to the const names.

There wasn't a particular reason, no! I thought that integers were just the common way to do enums like this in Go.

A nice benefit of this is that the filtering can be done with bitwise operations (to determine whether to log each level), but of course we could do that with a set of bools or something too: https://github.com/cdr/coder-doctor/blob/31cb1c565fc42be9b40d9c68e634625287086b0b/internal/filterwriter/filter.go#L42-L60

I suppose another benefit is that comparisons should be faster with integers, because then it's just a numeric equality instead of string comparison

Neato. Appreciate the context, and I'd agree with you here!

jawnsy · 2021-08-16T15:12:25Z

@johnstcn I wanted to get this initial version merged, can you take another look? I think you'd be the perfect reviewer for this since you've been working with the CLI more lately.

It's not complete and just checks the version for now, but it's going to be harder to iterate on this with the existing PR due to the size. If you want to test locally, you should be able to clone the branch and run:

$ go run . check kubernetes --verbosity=10
2021-08-16 15:10:20.462 [DEBUG] <version.go:67> selected coder version  {"requested": "1.21.0", "selected": "1.21.0"}
PASS Coder 1.21.0 supports Kubernetes 1.19.0 to 1.22.0 (server version 1.20.8-gke.900)

This will use your current kubectl context (or you can also specify flags like --context)

johnstcn · 2021-08-16T15:25:33Z

internal/api/types_test.go

+			emoji, err := state.Emoji()
+			assert.Success(t, "state.Emoji() error non-nil", err)
+			assert.True(t, "state.Emoji() is non-empty", len(emoji) > 0)
+			_ = state.MustEmoji()


❤️ this

johnstcn · 2021-08-16T15:32:25Z

internal/checks/kube/version.go

+
+type CoderVersionRequirement struct {
+	CoderVersion         *semver.Version
+	KubernetesVersionMin *semver.Version


nit: we can probably use a semver.Constraint instead of specifying new versions, but this works for now.
https://pkg.go.dev/github.com/Masterminds/semver#hdr-Checking_Version_Constraints

We can, but that API is a little more annoying to use because there's no Must variant. I suppose we could specify all of these as strings and parse at runtime?

We're probably better off doing as much as possible at compile-time!

johnstcn · 2021-08-16T15:40:36Z

internal/cmd/check/kubernetes/kubernetes.go

+
+	// TODO: this is pretty arbitrary, use a defined verbosity similar to
+	// kubectl
+	if verbosity > 5 {


Ha, I usually set --verbosity=99 or something if I see something like that and want to be extra-verbose.

kubectl has a really nicely-defined list, with criteria for what goes where -- I think we should adopt similar definitions, because otherwise (as a developer), it's difficult to know what level to log things, and it makes the level kinda useless for users: https://kubernetes.io/docs/reference/kubectl/_print/#kubectl-output-verbosity-and-debugging

johnstcn

Nice one!

./main check  kubernetes --verbosity 9999
2021-08-16 15:23:50.681 [DEBUG] <version.go:67> selected coder version  {"requested": "1.21.0", "selected": "1.21.0"}
PASS Coder 1.21.0 supports Kubernetes 1.19.0 to 1.22.0 (server version 1.20.8-gke.900)

johnstcn · 2021-08-16T15:41:57Z

internal/checks/kube/kubernetes.go

+func (k *KubernetesChecker) Run(ctx context.Context) error {
+	err := k.writer.WriteResult(k.CheckVersion(ctx))
+	if err != nil {
+		return err


Can we add more xerrors.Errorf(... about the place?

excellent point, done!

johnstcn · 2021-08-16T15:43:41Z

internal/checks/kube/version.go

+
+	kubernetesVersion, err := semver.NewVersion(versionInfo.GitVersion)
+	if err != nil {
+		return api.ErrorResult(checkName, "failed to parse server version", err)


Can we return a different message here than line 63 e.g. failed to parse git version info from server? Exact wording chef's choice.

excellent catch! changed the error on line 63 to read: failed to unmarshal version info instead, to clarify what it's actually doing

johnstcn · 2021-08-16T15:44:49Z

internal/cmd/check/kubernetes/kubernetes.go

+
+	overrides.CurrentContext, err = cmd.Flags().GetString(clientcmd.FlagContext)
+	if err != nil {
+		return nil, err


xerrors.Errorf(... and below

johnstcn · 2021-08-16T15:50:34Z

internal/humanwriter/human_test.go

+			var expected string
+			switch mode {
+			case humanwriter.OutputModeEmoji:
+				expected = "👍 human writer check test\n" +


nit: the pass and fail emoji are easy to confuse at a glance, suggest ✅ and ❌ for maximum distinguishability.

hmm, I don't feel strongly about it and suggest we try it as it is for a bit to see what we think -- we can always change it later

jawnsy · 2021-08-16T16:19:28Z

internal/checks/kube/version.go

+
+	// This uses the RESTClient rather than Discovery().ServerVersion()
+	// because the latter does not accept a context.
+	body, err := k.client.Discovery().RESTClient().Get().AbsPath("/version").Do(ctx).Raw()


on second thought, I think splitting this into a KubernetesVersionChecker struct might be useful, it could use a DiscoveryClient interface or RESTClient as its arg instead of a kubernetes.Interface -- that would make it easier to unit test https://pkg.go.dev/k8s.io/[email protected]/kubernetes/typed/discovery/v1#DiscoveryV1Interface

but I think that's something for later

jawnsy self-assigned this Aug 7, 2021

kylecarbs reviewed Aug 11, 2021

View reviewed changes

internal/cmd/root.go Show resolved Hide resolved

johnstcn reviewed Aug 11, 2021

View reviewed changes

internal/checks/kubernetes/version.go Outdated Show resolved Hide resolved

deansheather reviewed Aug 11, 2021

View reviewed changes

Emyrk reviewed Aug 13, 2021

View reviewed changes

internal/api/writer.go Show resolved Hide resolved

Jonathan Yu added 9 commits August 13, 2021 22:22

feat: initial implementation of coder-doctor

8a7c768

rename doctor -> coder-doctor

7152293

implement more of kubernetes check

215ee39

fix tests

fabea0d

refactor interfaces, add logging

04e12e3

add flags to control kubernetes settings

12836d2

wip

84882af

more tests

5a26ffc

add other resultwriter implementations

f2f9988

jawnsy force-pushed the jawnsy/ch15961/initial-implementation branch from 5fb3b2b to f2f9988 Compare August 14, 2021 02:17

update to 0.19.14

31cb1c5

kylecarbs reviewed Aug 14, 2021

View reviewed changes

jawnsy requested a review from johnstcn August 16, 2021 15:09

jawnsy marked this pull request as ready for review August 16, 2021 15:12

johnstcn reviewed Aug 16, 2021

View reviewed changes

johnstcn approved these changes Aug 16, 2021

View reviewed changes

jawnsy commented Aug 16, 2021

View reviewed changes

cian's code review

71f97fc

jawnsy merged commit 4a854f0 into main Aug 16, 2021

jawnsy deleted the jawnsy/ch15961/initial-implementation branch August 16, 2021 16:40

Conversation

jawnsy commented Aug 7, 2021

Uh oh!

shortcut-integration bot commented Aug 7, 2021

Uh oh!

Uh oh!

kylecarbs commented Aug 11, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jawnsy commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jawnsy commented Aug 16, 2021 •

edited

Loading