Skip to content
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.

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

Replace manual parsing of head comments #2

Closed
wants to merge 1 commit into from

Conversation

SgtCoDFish
Copy link
Member

@SgtCoDFish SgtCoDFish commented Jan 10, 2024

This leaves more to the YAML parser so we have less chance of accidentally introducing a bug or security issue in our own logic.

Also adds preservation of footer comments as a bonus!

In addition, this fixes klone init and klone add which are currently broken on main when run in a directory with no klone.yaml. That bug was caused by the comment preservation logic!

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2024
@inteon
Copy link
Member

inteon commented Jan 11, 2024

@SgtCoDFish
#4 added the necessary tests that will make sure this PR is tested for some of the yaml weirdness that can happen.

@SgtCoDFish
Copy link
Member Author

It seems like the "weirdness" is that there's a difference between the following:

# Comment

targets:
  target1:
    - ...

On the above, the comment is on the "document" and this PR will preserve it.

# Comment
targets:
  target1:
    - ...

On this one, the coment is on the targets node and this PR (as initially written) doesn't preserve it. The comment is in rawDocument.Content[0].Content[0].HeadComment and not rawDocument.HeadComment

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2024
This leaves more to the YAML parser so we have less chance of
accidentally introducing a bug or security issue in our own logic.

Also adds preservation of footer comments as a bonus!

Signed-off-by: Ashley Davis <[email protected]>
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sgtcodfish. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2024
@SgtCoDFish
Copy link
Member Author

The latest commit passes the tests by adding a newline to the test cases so that the comments are now doc comments. I dunno about this... it makes sense to only preserve doc comments to me, and I can see why the yaml parser would treat the other comments differently. I'm not militant about it though

@SgtCoDFish
Copy link
Member Author

I think it'd be better to use a YAML parser for this but I don't think it's worth the effort coming up with a better solution here. Closing.

@SgtCoDFish SgtCoDFish closed this Jan 15, 2024
@SgtCoDFish SgtCoDFish deleted the commentparse branch January 15, 2024 18:37
@inteon
Copy link
Member

inteon commented Jan 15, 2024

I think it'd be better to use a YAML parser for this but I don't think it's worth the effort coming up with a better solution here. Closing.

Thanks for testing and letting me know about the io.EOF error btw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants