fix: revert string parsing in configmap to not trim spaces

[Cali0707]

In #3200 we switched the configmap parser to trim spaces in strings. Unfortunately, this has broken the parsing of configmaps with multi-line embedded yamls in downstream repos like eventing (see failures in knative/eventing#8618). This PR reverts the change

Changes

• Do not trim spaces in configmap string parsing

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Cali0707]

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

[Cali0707]

/assign @dprotaso @evankanderson

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.61%. Comparing base (3eb1089) to head (7f71db5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3211      +/-   ##
==========================================
- Coverage   74.63%   74.61%   -0.03%     
==========================================
  Files         209      209              
  Lines       12022    12022              
==========================================
- Hits         8973     8970       -3     
- Misses       2770     2772       +2     
- Partials      279      280       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dprotaso]

Can you clarify the problem more?

The trim'ing just trims leading and trailing whitespace

[Cali0707]

Can you clarify the problem more?

The trim'ing just trims leading and trailing whitespace

Yeah, the issue we are seeing in eventing is that there are configmaps with data that looks like:

"channel-template-spec": "\tapiVersion: Foo/v1\n\tkind: Bar\n"

And since we trim the whitespace the "value" becomes:

"apiVersion: Foo/v1\tkind: Bar\n"

Resulting in an error that yaml: line 2: mapping values are not allowed in this context, when we try and convert the result from yaml to JSON (see https://github.com/knative/eventing/blob/main/pkg/reconciler/broker/config.go#L65)

Essentially, the problem is that if we try to use cm.AsString with a string that contains multiple lines of yaml, the subsequent parsing/manipulation we do of that now breaks

[dprotaso]

This honestly might be more of an issue with the fixture string in the test

eg. https://go.dev/play/p/wH7mgI5UXBO

[dprotaso]

I think I'd prefer eventing just make their own parser func to handle this YAML scenario.

The alternative would be to have all this extra validation everywhere. We'd have to trim the strings for whitespace then check if it's empty.

[Cali0707]

I think I'd prefer eventing just make their own parser func to handle this YAML scenario.

The alternative would be to have all this extra validation everywhere. We'd have to trim the strings for whitespace then check if it's empty.

That's fair, I'll close this then

/close

[knative-prow]

@Cali0707: Closed this PR.

Details

In response to this:

I think I'd prefer eventing just make their own parser func to handle this YAML scenario.

The alternative would be to have all this extra validation everywhere. We'd have to trim the strings for whitespace then check if it's empty.

That's fair, I'll close this then

/close

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-sigs/prow repository.

[evankanderson]

/lgtm
/approve