-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: support config file namespace validation #596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot for looking into this 👍
Did you actually use the namespace watcher changes you made? I could not find how you used it but I am on my phone only so maybe overlooked it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long time, I made a review but somehow got distracted and did not submit it 🤦
There are just some improvement ideas, but it is nearly good to go 👍
Just fyi - @zepatrik is on vacation until June 14th so reviews will probably take a bit longer :) I don't really feel that I have enough context to review the PR :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really sorry about the long review times.
This looks very good, I have some ideas for further improvement.
Thanks for the follow up @zepatrik ! Your feedback is very thorough and I appreciate the patience as I learn some Go basics. I will address some more of the advanced suggestions over the next few days and re-request you as a reviewer when complete. |
@zepatrik re-added you as a reviewer. Thanks again for taking the time for a thorough review. I learned a lot. Also - I think I will keep the fail-fast strategy for namespace file validation because I think it makes sense to approach the corrections one-by-one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this is nearly good to go ❤️
Good idea btw |
- support validation of namespaces in config files - support validation of a list of namespace files - modify file watcher to keep unparsable input - extract parser function
@zepatrik rebased and squashed together! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot ❤️
Related issue
#586
cc @zepatrik
Proposed changes
Adds support for parsing configuration files containing either a list of namespace objects or a reference to a location with namespace objects.
It extends the existing functionality. Here's example usage.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
I changed the way the namespace watcher works. This is because I wanted to reuse the code in watcher which determines the correct parser type and does an initial scrape of the target location. To make it possible to reuse watcher for validation, I had to leave the original source file contents inside the scraped file.