Skip to content

Conversation

@miabbott
Copy link
Member

In openshift/os#595 and openshift/os#599, I found myself mixed up with
the support of the tracker field. It's defined as a string, so
magically hoping that a slice of strings would be supported was
misguided.

Decided to try adding support for a new tracker_list field for those
times when one issue just doesn't cut it.

This seemed more straight-forward than trying to manually unmarshal
the YAML or trying to find all the denylist documents in use.

In openshift/os#595 and openshift/os#599, I found myself mixed up with
the support of the `tracker` field.  It's defined as a string, so
magically hoping that a slice of strings would be supported was
misguided.

Decided to try adding support for a new `tracker_list` field for those
times when one issue just doesn't cut it.

This seemed more straight-forward than trying to manually unmarshal
the YAML or trying to find all the denylist documents in use.
@openshift-ci
Copy link

openshift-ci bot commented Jul 30, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@miabbott
Copy link
Member Author

Looking for feedback if this is desirable or not.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I'm fine with this, but it's not clear we benefit from an explicit list, e.g. we can also use space-separated strings, or have one entry be canonical and the other a comment e.g.

Copy link
Contributor

@saqibali-2k saqibali-2k left a comment

Choose a reason for hiding this comment

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

The new functionality makes sense to me 👍

Comment on lines +281 to +282
Tracker string `yaml:"tracker"`
TrackerList []string `yaml:"tracker_list"`
Copy link
Contributor

@saqibali-2k saqibali-2k Aug 3, 2021

Choose a reason for hiding this comment

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

Suggested change
Tracker string `yaml:"tracker"`
TrackerList []string `yaml:"tracker_list"`
Tracker []string `yaml:"tracker"`

I think we can change Tracker to []string from string rather than adding a TrackerList which seems to be closer to the assumed functionality in the linked PRs. If we prefer to have a separate field, that works too!

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried the wholesale change from string to []string, but the YAML module is strict about the type (well, all of Golang is strictly typed...) so it blows up trying to unmarhsal the data:

cannot unmarshal !!seq into main.DenyListObj

See this example snippet I was playing with - https://play.golang.org/p/WxafbrQGuNe

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep you're right that functionality isn't present, ignore the above suggestion.

@miabbott
Copy link
Member Author

miabbott commented Aug 3, 2021

I'm fine with this, but it's not clear we benefit from an explicit list, e.g. we can also use space-separated strings, or have one entry be canonical and the other a comment e.g.

I like the idea of a single entry being canonical and having others as comments.

@miabbott
Copy link
Member Author

miabbott commented Aug 6, 2021

Going to withdraw this suggestion; let's just a use single canonical URL for the tracker and add additional links as comments.

@miabbott miabbott closed this Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants