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

cmd: Add support for glob matching #79

Merged
merged 7 commits into from
Jan 29, 2019
Merged

cmd: Add support for glob matching #79

merged 7 commits into from
Jan 29, 2019

Conversation

rliebz
Copy link
Contributor

@rliebz rliebz commented Jan 24, 2019

Related issue #66

Proposed changes

Adds support for the "glob" flavor, following the same patterns as "exact" and "regex".

Checklist

  • I have read the contributing guidelines
  • 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 signed the Developer's Certificate of Origin
    by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
    amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
    force push.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate) (Update documentation for glob support docs#102)

Further comments

scripts/test-e2e.sh Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Jan 24, 2019

This looks pretty good already! go-swagger is broken with go modules so the only way to get it working is to have keto in $GOPATH/src/github.com/ory/keto and disable go modules (or rather not set GO111MODULES=on) and then run it. If there are weird changes with the SDK (especially in the Go base) I'd rather not update them and would check this out myself - your changes should not impact the SDK anyways.

And yes, please update developer guide, especially the inconsistencies :)

@aeneasr
Copy link
Member

aeneasr commented Jan 24, 2019

Docs can be updated here: https://github.com/ory/docs/tree/master/docs/keto

@rliebz
Copy link
Contributor Author

rliebz commented Jan 24, 2019

On the correct path I still get errors from github.com/ory/x/metricsx. I tried checking out the latest version of that as well as the version in go.mod (v0.0.33) and still get the following error:

$ echo $GOPATH && pwd
/home/rob/Projects/go
/home/rob/Projects/go/src/github.com/ory/keto
$ GO111MODULE=off make swagger        
swagger generate spec -m -o ./docs/api.swagger.json
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:135:19: Config not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:137:28: NewWithConfig not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:113:28: NewWithConfig not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:113:62: Config not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:228:13: invalid operation: sw.Segment (variable of type github.com/segmentio/analytics-go.Client) has no field or method Enqueue
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:232:4: NewProperties not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:231:3: unknown field Properties in struct literal
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:242:23: Context not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:189:24: invalid operation: sw.Segment (variable of type github.com/segmentio/analytics-go.Client) has no field or method Enqueue
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:192:26: Properties not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:193:27: Context not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:158:21: invalid operation: sw.Segment (variable of type github.com/segmentio/analytics-go.Client) has no field or method Enqueue
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:160:22: NewTraits not declared by package analytics
/home/rob/Projects/go/src/github.com/ory/x/metricsx/middleware.go:170:24: Context not declared by package analytics
couldn't load packages due to errors: github.com/ory/x/metricsx
Makefile:12: recipe for target 'swagger' failed
make: *** [swagger] Error 1

I'm not sure what the issue is there, but it was just as easy for me to run sed against the file to update the generated output, so I went ahead and pushed that up. Let me know if there's issues with that approach.

Otherwise, I think this should be good to go.

@sum2000
Copy link
Contributor

sum2000 commented Jan 24, 2019

@rliebz You also need to add placeholder .go file inside glob folder and import that package inside rego/doc.go. See latest keto master ladon/rego subfolders. Here is my branch for reference (https://github.com/ory/keto/pull/77/commits)

@rliebz
Copy link
Contributor Author

rliebz commented Jan 24, 2019

@sum2000 Good catch, fixed.

@aeneasr
Copy link
Member

aeneasr commented Jan 28, 2019

This all looks superb, I added two comments in the respective PRs and once they're addressed this is good to go in master. Thank you for your work on this!

@aeneasr
Copy link
Member

aeneasr commented Jan 28, 2019

Oh, I overlooked your comment, I'll have to check that manually

@aeneasr
Copy link
Member

aeneasr commented Jan 28, 2019

The CI format error can be resolved via: https://github.com/ory/hydra/pull/1273/files#diff-1d37e48f9ceff6d8030570cd36286a61R25

@aeneasr
Copy link
Member

aeneasr commented Jan 28, 2019

For swagger, try this:

$ GO111MODULE=on go get ./...
$ GO111MODULE=on go mod vendor
$ GO111MODULE=off make swagger        
$ rm -rf ./vendor

@rliebz
Copy link
Contributor Author

rliebz commented Jan 28, 2019

@aeneasr Done.

Also I restructured the tests by moving the policies next to their relevant tests for readability, and using more granular, more specifically named test cases to keep hints on the console rather than just in code comments.

@sum2000
Copy link
Contributor

sum2000 commented Jan 28, 2019

@rliebz you forgot to import glob in rego/doc.go. If you merge with the latest master you will have that file inside rego folder.

Signed-off-by: Rob Liebowitz <[email protected]>
@rliebz
Copy link
Contributor Author

rliebz commented Jan 28, 2019

@sum2000 done.

I don't want to play around with CI as part of this change set, but it may make sense to add a regression test to make sure the issue in #73 doesn't pop back up.

@aeneasr
Copy link
Member

aeneasr commented Jan 29, 2019

Thank you for your hard work!

@aeneasr aeneasr merged commit 7798442 into ory:master Jan 29, 2019
@aeneasr
Copy link
Member

aeneasr commented Jan 29, 2019

There are some releases scheduled soon, I'm not sure if I can get it done this week though but I'll try (we're currently raising money, hiring, moving office, incepting hive and ory cloud, so I have insane workloads).

@rliebz rliebz deleted the glob branch January 29, 2019 14:34
@sum2000
Copy link
Contributor

sum2000 commented Feb 1, 2019

@rliebz I will try to add regression tests for these by the weekend, just been super busy at work.

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.

3 participants