Skip to content

Conversation

@bluekeyes
Copy link
Contributor

Do not treat empty input as an invalid remote reference, since apps may
use an empty file as an on/off switch. Also add some tests for parsing,
since there are several edge cases now.

Do not treat empty input as an invalid remote reference, since apps may
use an empty file as an on/off switch. Also add some tests for parsing,
since there are several edge cases now.
@bluekeyes bluekeyes requested a review from a team July 30, 2021 21:25
@bluekeyes
Copy link
Contributor Author

bluekeyes commented Jul 30, 2021

This is messier than I'd like, but I couldn't think of a better way to detect empty files (which may be all comments or whitespace, and thus not literally empty) without parsing the input twice.

Edit: I fixed this by making Remote the only required field, matching what Policy Bot does. That reduced the number of pointers and made everything simpler. The other side of this is in #95, which fixes the loading behavior when these optional fields are empty.

Copy link
Member

@jmcampanini jmcampanini left a comment

Choose a reason for hiding this comment

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

this seems better than what it is currently, so +1

just some non-blocking questions

Ref *string `yaml:"ref"`
}

if err := yaml.UnmarshalStrict(b, &maybeRef); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

curious: why not just Unmarshal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are any fields that aren't part of the RemoteRef structure, the parser assumes the file is not a remote reference. Using UnmarshalStrict returns an error if there are unknown fields.

@bluekeyes bluekeyes merged commit 69cf8c8 into develop Aug 3, 2021
@bluekeyes bluekeyes deleted the bkeyes/fix-remote-parser branch August 3, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants