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

feat: support config file namespace validation #596

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

cramja
Copy link
Contributor

@cramja cramja commented May 15, 2021

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.

### VALID
./keto namespace validate -c config.yaml config2.yaml

./keto namespace validate namespace.yaml ns2.yaml

# namespaces and configs are all validated
./keto namespace validate namespace.yaml ns2.yaml -c config.yaml

### INVALID

# interprets config.yaml as a namespace file
./keto namespace validate config.yaml

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    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.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

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.

@CLAassistant
Copy link

CLAassistant commented May 15, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@zepatrik zepatrik left a 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 😅

cmd/namespace/validate.go Outdated Show resolved Hide resolved
internal/driver/config/namespace_watcher.go Outdated Show resolved Hide resolved
internal/driver/config/namespace_watcher.go Show resolved Hide resolved
internal/driver/config/namespace_watcher.go Outdated Show resolved Hide resolved
@cramja cramja requested a review from zepatrik May 20, 2021 04:41
Copy link
Member

@zepatrik zepatrik left a 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 👍

cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Show resolved Hide resolved
cmd/namespace/validate_test.go Outdated Show resolved Hide resolved
cmd/namespace/validate_test.go Outdated Show resolved Hide resolved
internal/driver/config/namespace_watcher.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Jun 3, 2021

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 :/

Copy link
Member

@zepatrik zepatrik left a 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.

cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate_test.go Outdated Show resolved Hide resolved
cmd/namespace/validate_test.go Outdated Show resolved Hide resolved
cmd/namespace/validate_test.go Show resolved Hide resolved
internal/driver/config/namespace_watcher.go Outdated Show resolved Hide resolved
@cramja
Copy link
Contributor Author

cramja commented Jun 17, 2021

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.

@cramja cramja requested a review from zepatrik June 23, 2021 16:30
@cramja
Copy link
Contributor Author

cramja commented Jun 23, 2021

@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.

Copy link
Member

@zepatrik zepatrik left a 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 ❤️

cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate.go Outdated Show resolved Hide resolved
cmd/namespace/validate_test.go Outdated Show resolved Hide resolved
@zepatrik
Copy link
Member

zepatrik commented Jun 25, 2021

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.

Good idea btw
Can you then please rebase/merge current master into this?

- 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
@cramja cramja requested a review from zepatrik June 28, 2021 15:57
@cramja
Copy link
Contributor Author

cramja commented Jun 28, 2021

@zepatrik rebased and squashed together!

Copy link
Member

@zepatrik zepatrik left a 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 ❤️

@zepatrik zepatrik merged commit f4253b8 into ory:master Jun 29, 2021
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