Fix: Enforce presence of Key and Value keys on field tag definition#266
Conversation
mlevesquedion
force-pushed
the
fix-field-tag-value-bug
branch
from
8b2dc5a to
22557f3
Compare
mlevesquedion
requested review from
PurelyApplied,
benhxy and
vinayakankugoyal
| @@ -147,8 +147,9 @@ type fieldTagMatcher struct { | |||
|
|
|||
| // this type uses the default unmarshaller and mirrors configuration key-value pairs | |||
| type rawFieldTagMatcher struct { | |||
There was a problem hiding this comment.
Wont this allow to be parsed?
FieldTags:
- Key: datapolicy
Val: cert
Value: password
There was a problem hiding this comment.
That is correct. In that case we should probably do like we are doing with e.g. Package and PackageRE, and only allow one or the other to be set. Although, in my opinion there is little value in having both Val and Value, so I think we may actually want to keep only one of them. WDYT?
There was a problem hiding this comment.
Yeah lets just keep Value and treat setting Val as an error. Being strict while parsing the config is totally fine IMO.
There was a problem hiding this comment.
@PurelyApplied, @benhxy, thoughts on this?
There was a problem hiding this comment.
Oh, I had a WIP branch that resolved some of this with RawMessage and switch statements. *dig dig dig*
#271 opened as draft. I hadn't added tests yet, but it hits some of the concerns you've voiced. There's also the issue where, if we want to be case insensitive, you could theoretically add all of key, Key, KEY, etc and have collisions that way while still being valid YAML. #271 pokes at the other matchers, too, since I wasn't a huge fan of that Valid Keys slice.
That is correct. In that case we should probably do like we are doing with e.g. Package and PackageRE, and only allow one or the other to be set. Although, in my opinion there is little value in having both Val and Value, so I think we may actually want to keep only one of them. WDYT?
I'd be +1 to only retaining Value. I think we'd kept Val because we didn't want to rewrite existing test configs. But we're v0.x. We can make a config-breaking change here.
| wantErr: false, | ||
| }, | ||
| { | ||
| desc: "valid field tag config using value", |
There was a problem hiding this comment.
can we add a case where both Val and Value are set?
There was a problem hiding this comment.
This test wont be necessary if we only allow Value and not Val.
mlevesquedion
changed the title
| - name: Check YAML | ||
| uses: ibiqlik/action-yamllint@v3 | ||
| run: | | ||
| yamllint . |
There was a problem hiding this comment.
I had to change this because the action was reporting an error, while running yamllint locally did not. As it turns out, Github Actions runners have yamllint installed out of the box, so we don't need to use a third-party action.
PurelyApplied
left a comment
PurelyApplied
left a comment
There was a problem hiding this comment.
LGTM. There's still plenty of cleanup that could be done, but that could wait for a future PR / until I clean up #271.
We still have a case insensitivity issue that could use a test, e.g.
FieldTags:
- Key: levee
key: also-levee
Value: source
value: ExtraSourcey
mlevesquedion
changed the title
3 participants
I ran into this issue while writing the quickstart user guide (#265).
Originally when I was writing the config, I wrote:
Currently, we "allow"
Valueas a "valid" key, but in reality, if you useValueinstead ofVal, theValwill be empty and your matcher won't actually match anything. In fact, we don't check that the fields are present, only that if some fields are present, they are one ofkey, val, value.So e.g. if you write:
the code will complain about
Foobeing an invalid key, but if you write this:it won't complain about the missing
Valfield, but your matcher won't be able to match anything. (Unless your value really is empty, but I don't think we should support that use case.)This PR fixes the issue by checking that the parsed
KeyandValare non-empty strings, and producing an error if that isn't the case.