Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

bug: manifest parser sees spaces in comments as multiple results #271

Open
scottd018 opened this issue Feb 9, 2022 · 1 comment
Open
Assignees
Labels
bug Something isn't working

Comments

@scottd018
Copy link
Contributor

scottd018 commented Feb 9, 2022

Consider the following manifest:

---
# +operator-builder:resource:collectionField=provider,value="aws",include
apiVersion: v1
kind: ConfigMap
metadata:
  name: contour-config-test-collection-field
  namespace: ingress-system  # +operator-builder:field:name=namespace,default=ingress-system,type=string
data:
  this: "serves no purpose other than to test resource markers on collection fields"
---
# +operator-builder:resource:collectionField=provider,value="aws",include
# Note: Comments with spaces can cause problems with parsing.  Leave this here 
# for functional testing purposes.

# The space above is actually what causes the problem.
apiVersion: v1
kind: ConfigMap
metadata:
  name: contour-config-test-parse-comment
  namespace: ingress-system  # +operator-builder:field:name=namespace,default=ingress-system,type=string
data:
  this: "serves no purpose other than to test comment spaces for resource markers on collection fields"

With a space in the comment, this will actually return multiple marker results for the contour-config-test-parse-comment ConfigMap. While this is not necessarily an issue if we handle the result properly, the marker package should only return one result as there is only one marker.

UPDATE: this is actually in the manifest parser, not the marker parser, as the static content while parsing looks like this:

(this is from a debug console)

"\n# +operator-builder:resource:collectionField=provider,value=\"aws\",include\napiVersion: v1\nkind: ConfigMap\nmetadata:\n    name: contour-config-test-collection-field\n    namespace: !!var parent.Spec.Namespace # controlled by field: namespace\ndata:\n    this: \"serves no purpose other than to test resource markers on collection fields\"\n\n# +operator-builder:resource:collectionField=provider,value=\"aws\",include\n# Note: Comments with spaces can cause problems with parsing.  Leave this here \n# for functional testing purposes."
@scottd018 scottd018 added the bug Something isn't working label Feb 9, 2022
@scottd018 scottd018 changed the title bug: marker parser sees spaces in comments as multiple results bug: manifest parser sees spaces in comments as multiple results Feb 9, 2022
@scottd018
Copy link
Contributor Author

This appears to be a bug in the upstream yaml.v3 package. Given the above set of manifests, we get:

Manifest 0:

---
# +operator-builder:resource:collectionField=provider,value="aws",include
apiVersion: v1
kind: ConfigMap
metadata:
  name: contour-config-test-collection-field
  namespace: ingress-system  # +operator-builder:field:name=namespace,default=ingress-system,type=string
data:
  this: "serves no purpose other than to test resource markers on collection fields"
---
# +operator-builder:resource:collectionField=provider,value="aws",include
# Note: Comments with spaces can cause problems with parsing.  Leave this here 
# for functional testing purposes.

Manifest 1:

# The space above is actually what causes the problem.
apiVersion: v1
kind: ConfigMap
metadata:
  name: contour-config-test-parse-comment
  namespace: ingress-system  # +operator-builder:field:name=namespace,default=ingress-system,type=string
data:
  this: "serves no purpose other than to test comment spaces for resource markers on collection fields"

The newline character causes the yaml marshaler to see the resource marker as a foot comment to the first manifest and takes the newline as the start of a new manifest.

lander2k2 pushed a commit that referenced this issue Feb 11, 2022
…ng (#263)

* fix: Fixes #236 glob pattern for doublestar

Signed-off-by: Dustin Scott <[email protected]>

* fix: fix panic when workload references collectionField

This commit addresses a panic situation when the workload itself references a
collection field, however the workload itself is not using a collection field
as an input to any of its fields.

Signed-off-by: Dustin Scott <[email protected]>

* test: add functional test for panic situation

Signed-off-by: Dustin Scott <[email protected]>

* fix: Fixes #256, added ability to associate all fields after processing

Signed-off-by: Dustin Scott <[email protected]>

* fix: handle array to interface conversion more intelligently

Signed-off-by: Dustin Scott <[email protected]>

* fix: handle case of comments below markers being seen as markers

Signed-off-by: Dustin Scott <[email protected]>

* test: use namespaced resource instead of cluster scoped resource

Signed-off-by: Dustin Scott <[email protected]>

* chore: fix linter error by removing linting item for TODO

Signed-off-by: Dustin Scott <[email protected]>

* chore: prepare test case for #271

This simply is a reminder to prepare for a test case for #271.  It does not
include an empty line, as an empty line would break the test case.  Once
in order to produce our functional test to account for this.

Signed-off-by: Dustin Scott <[email protected]>

* fix: Fixes #272, unmarshal struct with omitted fields

This adds the 'omitempty' tag to all fields within the API.  This allows the
unmarshal to unmarshal with empty fields.  Without the 'omitempty' tag, the
unmarshal event results in zero values before it can be processed by the CRD
which is there to set the default values.

Signed-off-by: Dustin Scott <[email protected]>

* test: remove conflict between quota and deployments without resources

Signed-off-by: Dustin Scott <[email protected]>

* test: fix logging test when controller is not deployed in cluster

Signed-off-by: Dustin Scott <[email protected]>

* test: fix missing os path in generated file

Signed-off-by: Dustin Scott <[email protected]>

* test: adjusted resources for resource quota

Signed-off-by: Dustin Scott <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants