Add ability to disable test on specified namespaces

[psibi]

This features adds the ability to disable tests on the specified
namespaces.

Specific usage example:

kube-score score --ignore-namespace=logging,minio -

This usecase is specifically for ignore the checks on namespace like
istio-system which isn't managed by us.

Also, this is my first time writing Go code. So I could be totally doing things wrong/non idiomatic way.

RELNOTE: Add ability to disable test on specified namespaces by using --ignore-namespace option

[zegl]

Hey, welcome to Go and kube-score!

This feature can make sense, but likely not in it's current form.

kube-score already has functionality to ignore an object, and I would not want to merge something that didn't work in the same way. Marking the object as skipped, and adding a comment makes it clear to the user why this object was skipped, and it's integrated with the -v flag, so that you can see skipped objects when the verbosity is increased.

kube-score/scorecard/scorecard.go

Lines 92 to 103 in cf480a5

	func (so *ScoredObject) Add(ts TestScore, check ks.Check, locationer ks.FileLocationer) {
	ts.Check = check
	so.FileLocation = locationer.FileLocation()
	// This test is ignored (via annotations), don't save the score
	if _, ok := so.ignoredChecks[check.ID]; ok {
	ts.Skipped = true
	ts.Comments = []TestScoreComment{{Summary: fmt.Sprintf("Skipped because %s is ignored", check.ID)}}
	}
	so.Checks = append(so.Checks, ts)
	}

Integrating the ignoring here, instead of adding a layer (and lots of code to handle it) would make a lot of sense. The *ScoredObject contains a ObjectMeta.Namespace which you can use to compare the ignorelist with.

When contributing to a OSS project, I'd recommend to open an issue first, to discuss the feature and to get feedback from the maintainers before starting work on an implementation. I think it would have saved you some time here, as I could have given you some pointers.

[psibi]

@zegl

I'd recommend to open an issue first, to discuss the feature and to get feedback from the maintainers before starting work on an implementation. I think it would have saved you some time here, as I could have given you some pointers.

Thanks for the feedback, I was considering opening an issue - but I didn't considering adding the feature myself initially. Will keep that in mind! :-) And yes, that would have saved some time for me.

Integrating the ignoring here, instead of adding a layer (and lots of code to handle it) would make a lot of sense.

I will see if I can update the code to be ignored with a comment. Thanks!

[psibi]

@zegl I have updated the code based on your feedback and I think this is ready for review. This uses the existing skipped objects machinery to ignore namespace.

[psibi]

@zegl Gentle ping. :-)

[zegl]

I think that it would be nicer to pass the configuration through NewObject instead, similar to how it's done for useIgnoreChecksAnnotation .

An additional pro of this is that it only requires the fix of that caller, instead of off in all calls to Add. I also think that it would be cleaner to only send a the set of ignored namespaces, instead of a reference to the whole configuration object.

func (s Scorecard) NewObject(typeMeta metav1.TypeMeta, objectMeta metav1.ObjectMeta, useIgnoreChecksAnnotation bool, ignoredNamespaces map[string]struct{}) *ScoredObject

[zegl]

Suggested change

	ts.Comments = []TestScoreComment{{Summary: fmt.Sprintf("Skipped because %s namespace is ignored", so.ObjectMeta.Namespace)}}
	ts.Comments = []TestScoreComment{{Summary: fmt.Sprintf("Skipped because the namespace %s is ignored", so.ObjectMeta.Namespace)}}

[zegl]

I've left a review with suggestions on how to make this code more straight forward.

I'd also like to remind you about kube-scores contribution guidelines. Especially when it comes to the commit messages. A good commit message for this change would be score: allow to disable scoring of objects in specific namespaces or something along those lines, the commits should also be squashed to a single one.

[psibi]

@zegl Thanks for the review! Unfortunately this took quite longer than expected for me to come back and I see that branches have diverged quite a bit. I'm opening a new PR: #365 which supersedes this and which (hopefully) addresses your feedback. I'm closing this MR.