add enable-feature command

[MatousJobanek]

SANDBOX-661

a command to enable a feature for the given space

EDIT: pushed another commit that checks if the requested feature is in the list of the supported toggle features in ToolchainConfig.

[codecov]

Codecov Report

Attention: Patch coverage is 81.03448% with 11 lines in your changes missing coverage. Please review.

Project coverage is 66.79%. Comparing base (ffc4214) to head (b33810a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   66.43%   66.79%   +0.36%     
==========================================
  Files          41       42       +1     
  Lines        2276     2334      +58     
==========================================
+ Hits         1512     1559      +47     
- Misses        584      591       +7     
- Partials      180      184       +4     

Files	Coverage Δ
pkg/cmd/root.go	88.57% <100.00%> (+0.33%)	⬆️
pkg/cmd/enable-feature.go	80.70% <80.70%> (ø)

[mfrancisc]

Looks great 🥇

I have only few minor comments.

I was wondering if we want to support list of features and/or list of spaces, but I believe running the command multiple times is more then fine, considering that execution time is very short.

[mfrancisc]

I think we can always update the annotations, the result shouldn't change

Suggested change

	if data.alreadyEnabled != "" {
	space.Annotations = map[string]string{
	toolchainv1alpha1.FeatureToggleNameAnnotationKey: data.alreadyEnabled,
	}
	}
	space.Annotations = map[string]string{
	toolchainv1alpha1.FeatureToggleNameAnnotationKey: data.alreadyEnabled,
	}

[alexeykazakov]

I would keep it as is. Just to make sure we do not touch the annotation map at all if the initial feature list is not set. There is still a difference between an empty map and nil.

[MatousJobanek]

yes, as Alexey mentioned, it's not the same. We need to make sure that the logic will work when annotations are no initialized (the map is nil). If we don't handle such a case properly, then we could end up with panic trying to modify a map that is nil.

[mfrancisc]

I see, but applying my suggestion the tests are still passing, so I'm wondering if we are missing some specific case that sets the annotation to nil to cover this?

[MatousJobanek]

It's the other way around. Your version puts lower requirements on the code than the current test.
Imagine that we don't have these 3 lines of the code

ksctl/pkg/cmd/enable-feature.go

Lines 95 to 97 in b33810a

	if space.Annotations == nil {
	space.Annotations = map[string]string{}
	}

Your version of the tests will pass, but the current version will fail

[mfrancisc]

Ok I see now, as you explained in our 1:1 this check makes so that we test the nil scenario by not setting any annotation in the setup part of the test and leaving it as nil in the first iterartion. Thanks for explaining and sorry fo the confusion.

[mfrancisc]

same here, we could always update the annotations I believe

Suggested change

	if alreadyEnabled != "" {
	space.Annotations = map[string]string{
	toolchainv1alpha1.FeatureToggleNameAnnotationKey: alreadyEnabled,
	}
	}
	space.Annotations = map[string]string{
	toolchainv1alpha1.FeatureToggleNameAnnotationKey: alreadyEnabled,
	}

[MatousJobanek]

The same - we need to verify the case when there is no annotation provided at all (the map is nil).

[mfrancisc]

same here, when I try this change the tests are still passing

[alexeykazakov]

Nice!
Now we need a command for disabling a feature :) I didn't notice that it's already there: #45 Nice!

[mfrancisc]

Looks great 👍