Sync default properties with upstream

[skabashnyuk]

Removed some defaults to be closer to upstream configuration

• CHE_PREDEFINED_STACKS_RELOAD__ON__START - not used in Che 7
• CHE_WORKSPACE_AUTO_START - wrong parameter, has to be CHE_WORKSPACE_AUTO__START, upstream default value - true
• CHE_INFRA_KUBERNETES_WORKSPACE__UNRECOVERABLE__EVENTS - upstream value FailedMount,FailedScheduling,MountVolume.SetUp failed,Failed to pull image,FailedCreate
• CHE_LIMITS_WORKSPACE_IDLE_TIMEOUT - upstream value 1800000
• CHE_INFRA_KUBERNETES_SERVICE__ACCOUNT__NAME - Not changed since it has NULL value in upstream. To remove it we need to investigate if it's safe to do or not.

Refer to eclipse-che/che#14614

[davidfestal]

In fact these values in the custom config are quite old and were put there by the operator long ago, at the beginning. In fact even the custom config map (which is a very weird way to allow customizing Che properties) would disappear after issue eclipse-che/che#14635 is implemented.

And in the context of issue eclipse-che/che#14635, the custom config map will be removed from all the namespaces managed by the che-operator, and the existing CM values would be added in the new overrideCheProperties field of the CheCluster custom resource.

So if some of the values that were added when creating the custom config map, please comment on issue eclipse-che/che#14635 that they should not be pul into the overrideCheProperties field of the CheCluster custom resource.

[davidfestal]

cc @tomgeorge who nearly finished implementing issue eclipse-che/che#14635

[skabashnyuk]

I think in context eclipse-che/che#14614 we are talking about new installations

[sleshchenko]

If it's important only for new installation - then LGTM

[davidfestal]

I think in context eclipse/che#14614 we are talking about new installations
If it's important only for new installation - then LGTM

OK. But we should proably think about the update of existing installations made from previous operator releases, that will still hold old Che server properties in either their custom config mapor the newoverrideChePropertiesCR field. IMO the automatic migration from thecustomconfig map to the newoverrideCheProperties` CR field would be the best moment to automatically update old obsolete Che properties that are not expected anymore.

[davidfestal]

Approving this PR in the meantime.

[skabashnyuk]

СС @l0rd

[l0rd]

I think that it may be a misunderstanding. When I said that the operator/helm chart should not override the defaults I meant that they should not change the default che.properties value. Hence I think that in this case it would be better to explicitly set 30 minutes.

[skabashnyuk]

CC @sleshchenko @davidfestal I've made changes according to the latest @l0rd 's comment #86 (comment)

[davidfestal]

I think that it may be a misunderstanding. When I said that the operator/helm chart should not override the defaults I meant that they should not change the default che.properties value. Hence I think that in this case it would be better to explicitly set 30 minutes.

CC @sleshchenko @davidfestal I've made changes according to the latest @l0rd 's comment

@l0rd @skabashnyuk

It seems to me that both changes would have the same result : if the che.properties file in the Che server contains a 30 minutes idle timeout, it will be automatically used if we remove the corresponding value in the custom config map. I don't see how setting it explicitly in the custom config map adds value here.

Ideally we should tend to have an empty custom config map by default. The upstream default properties contained in the che.properties should be sufficient to run without overriding anything.

IMO adding a non-empty custom config map by default at installation is a confusing design error from the beginning.

[skabashnyuk]

@davidfestal @sleshchenko I've removed some defaults except one. This will make che-operator configuration is close to Che upstream. Are you ok with that?

[davidfestal]

Perfect !