jsondataLimiitSetmap set default to -1 seems empty map breaks downstream

[cr22rc]

Signed-off-by: rickr cr22rc@users.noreply.github.com

Fixes #

Proposed Changes

Release Note

Docs

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cr22rc
To complete the pull request process, please assign slinkydeveloper after the PR has been reviewed.
You can assign the PR to them by writing /assign @slinkydeveloper in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Merging #4893 (d5c317a) into master (78ee789) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4893   +/-   ##
=======================================
  Coverage   81.43%   81.43%           
=======================================
  Files         292      292           
  Lines        8340     8340           
=======================================
  Hits         6792     6792           
  Misses       1142     1142           
  Partials      406      406           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ee789...b93575b. Read the comment docs.

[antoineco]

I already commented on that in the previous PR but it wasn't addressed. There is no _example key in this ConfigMap, so this needs to be removed, otherwise a checksum check will always fail against this ConfigMap.

[lberk]

if -1 is the default and corresponds to no limit, why not have an integer field that isn't specified by default and only has an _example entry?

[cr22rc]

This was modeled after https://github.com/knative/eventing/blob/master/config/core/configmaps/default-channel.yaml
Why should this be any different here ?

[antoineco]

default-ch-config takes an entire YAML (structured) object as a value, which is harder to default since you need to make assumptions on what types of channel are available in the cluster.

For this feature, we are talking about simple keys/values, so the pattern below sounds much more suitable to me:

data:
  _example: |
    ################################
    #                              #
    #    EXAMPLE CONFIGURATION     #
    #                              #
    ################################

    # Max number of bytes allowed to be sent for message excluding any base64 decoding.
    dataMaxSize: -1

  dataMaxSize: 4096

The _example key is just a nice to have, but it's useful for users if the ConfigMap is deployed by default. (example)

[cr22rc]

@antoineco thx for that explanation. Due to other work I probably at this time won't get to make these changes before this coming release

[cr22rc]

@lberk Let me understand this. The breakage is on something that is downstream your vendor( redhat). By downstream you see this that as a later version of knative? Private changes?

[lberk]

@cr22rc the breakage is running a nightly CI with (no changes) on openshift. regardless of where the issue is reported and occurring from, we're less than a week outside of a release and as @antoineco has pointed out, there are still components of the review that haven't been addressed. It seems odd to me that we're going to rush to push in an api change to a v1beta2 resource a week out from a release, and then drop any bugs related to it which means we'll be stuck with them over a release boundary.

EDIT:
Specifically, I'm concerned about the maintenance burden of this (and the original change) this close to the release and the open review comments still not addressed.

[cr22rc]

@lberk This was merged. There were no issues found in KNeventing tests. It appeared to me only one in 103 of the openshift test failed. This must be a unique use case then. I think the proper way to handle this in opensource if you depend on it you need to contribute tests to assure it does not break.

[lionelvillard]

There are lot of interesting comments but I would like to stay focus on why it's breaking downstream, when running in OpenShift. The faulty PR has been merged at least one week before release exactly for the reason that any PRs can break downstream.

Going back to this issue, the value in the configmap is NOT empty. It looks like a bunch of YAML comments but it is not: it's a string. So something is removing those comments in your pipeline @lberk. Could that be possible?

All other issues, like _example, checksum, nested config, etc... yes I agree this can be done better but they are not the cause of the crash seen downstream (AFAICT so far).

[lionelvillard]

BTW if my assessment is correct (a big if), this PR should fix the crash

[lberk]

[...] I would like to stay focus on why it's breaking downstream, when running in OpenShift. The faulty PR has been merged at least one week before release exactly for the reason that any PRs can break downstream.

Thanks Lionel. I appreciate the constructive approach here.

Going back to this issue, the value in the configmap is NOT empty. It looks like a bunch of YAML comments but it is not: it's a string. So something is removing those comments in your pipeline @lberk. Could that be possible?

Possible, but at the same time I think this is indicative of the original patch. This should have been a simple key/value pair (as @antoineco has commented in this PR). Yet it accepts a string regardless if it's a comment or not.

All other issues, like _example, checksum, nested config, etc... yes I agree this can be done better but they are not the cause of
the crash seen downstream (AFAICT so far).

Part of the problem is that there is this level of cleanup mentioned on the original PR, that we'll have to carry forward across releases. In an effort to move forward positively, not burden the community, while getting this change across I've opened #4894

I'd suggest closing this PR and moving to use that if possible which includes a variety of the suggested cleanups

[antoineco]

Superseded by #4894
I think this can be closed?

[knative-prow-robot]

@cr22rc: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.