Add E2E test for default channel#1782
Conversation
knative-prow-robot
added
area/test-and-release
| defaultChannelConfig, hasDefault := configMap.Data[channelDefaulterKey] | ||
| config := &defaultchannel.Config{} | ||
| if hasDefault { | ||
| if err := yaml.Unmarshal([]byte(defaultChannelConfig), config); err != nil { |
There was a problem hiding this comment.
look at the channelDefaulter.UpdateConfigMap... we are converting the yaml to json, and then doing some update there... I think there was a problem with the casing of the keys otherwise...
BTW, is that something we can call, instead of duplicating the logic here? If not, no worries...
There was a problem hiding this comment.
We'll have to update the configmap ref first, then call the UpdateConfigMap method, and seems we don't have existed code doing the former.
And I feel the channelDefaulter is more for internal implementation, the only interesting thing to end user is the configmap itself.
So I didn't change anything here..
|
|
||
| // updateDefaultChannelCM will update the default channel configmap | ||
| func updateDefaultChannelCM(client *common.Client, updateConfig func(config *defaultchannel.Config)) error { | ||
| systemNamespace := resources.SystemNamespace |
There was a problem hiding this comment.
Do you need to do all these programmatic reading and updating of the configMap? and then call the defaulter UpdateConfigMap? Can't you just create a ConfigMap object in memory and set the appropriate value to a key in the Data field?
No need to do it now, but maybe when you add the thread-safety thing...
There was a problem hiding this comment.
Yeah I can do that, but I think we still cannot get rid of the yaml.Unmarshal and yaml.Marshal stuff, since it's not safe to directly change yaml, even in test.
|
/lgtm |
|
/cc @grantr |
grantr
left a comment
grantr
left a comment
There was a problem hiding this comment.
Thanks for writing this test @Fredy-Z!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Fredy-Z, grantr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
5 participants
Fixes #1711
Proposed Changes
Channel. The E2E test covers the cluster-wide and namespace-wide defaulters.Release Note
/cc @nachocano