Skip to content

jsondataLimiitSetmap set default to -1 seems empty map breaks downstream #4893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cr22rc wants to merge 1 commit into from
Closed

jsondataLimiitSetmap set default to -1 seems empty map breaks downstream #4893

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/core/configmaps/default-pingsource.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ metadata:
knative.dev/example-checksum: "6eaeecba"
Copy link
Copy Markdown
Contributor

@antoineco antoineco Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data:
ping-config: |
# dataMaxSize: 10 # Max number of bytes allowed to be sent for message excluding any base64 decoding.
Copy link
Copy Markdown
Member

@lberk lberk Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@cr22rc cr22rc Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@antoineco antoineco Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@cr22rc cr22rc Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

# Default is no limit set for data
dataMaxSize: -1 # Max number of bytes allowed to be sent for message excluding any base64 decoding.
# -1 For no limit