Skip to content

Comments

#614 add casbin for authorization#787

Merged
peterbourgon merged 3 commits intogo-kit:masterfrom
suekto-andreas:master
Nov 8, 2018
Merged

#614 add casbin for authorization#787
peterbourgon merged 3 commits intogo-kit:masterfrom
suekto-andreas:master

Conversation

@suekto-andreas
Copy link
Contributor

Enhancement #614

Add into the middleware: casbin

Design to be review are

  1. the arguments to be pass to NewEnforcer
  2. is auth directory the right place to put casbin
    any further feedback please help to add, thanks !

Disclaimer -- testdata are actually taken from casbin repository test

@suekto-andreas
Copy link
Contributor Author

Hi @peterbourgon, the tests failed on metrics/pcp - TestInconsistentLabelCardinality, it should be unrelated with this PR (auth/casbin)

can help to advise how should we continue ?

@peterbourgon
Copy link
Member

Oh cool! Ignore the error, I'll fix it up.

Primary review comment right now, please fix up all the doc comments to be valid English sentences with capitalization and punctuation, word-wrapped at 80 columns.

@peterbourgon
Copy link
Member

If you rebase the test errors should go away.

@suekto-andreas
Copy link
Contributor Author

thanks @peterbourgon, the pcp is good now but the circleci got err on sd/etcdv3 integration test part

const (
// CasbinModelContextKey holds the key to store the access control model
// in context, it can be a path to configuration file or a casbin/model
// Model
Copy link
Member

Choose a reason for hiding this comment

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

Still need punctuation etc. on all doc comments like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

submitted and update, would it be like that ? sorry kinda new into this.

bf51e5e

@peterbourgon peterbourgon merged commit da7ac23 into go-kit:master Nov 8, 2018
@peterbourgon
Copy link
Member

Thanks!

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.

2 participants