refactor(auth/token): Break out token roles functionality #96
Conversation
I wanted to get a head start on this while the token roles stuff was fresh in my mind! I've added two new classes: ClientFeature and TokenRoles. The former is a wrapper around the client for the basic API calls (post, read, list and delete). The later inherits the former and is where the new token roles functionality lives. Also added further checks to current tests. Motivation for changes is discussed in hvac#95. BREAKING CHANGES: * token_role(...) -> token_roles.read(...) * list_token_roles(...) -> token_roles.list(...) * create_token_role(...) -> token_roles.write(...) * delete_token_role(...) -> token_roles.delete(...)
|
Hey @ianunruh, Can I please get some feedback on this? It's my first attempt at tackling the new API design you proposed in #95. If you think it looks good I can start working on applying the same strategy to other feature sections. I'd like to do as much as I can this month as part of 24pullrequests. P.S. If we go with a feature branch for the API changes let me know what the target branch should be and I'll re-target this PR. |
There was a problem hiding this comment.
Overall the API looks good. I'm wondering if removing these methods from the "v1" client is a good idea in master. How would you feel about making a new "v2" client with the new API instead? The current v1 API is pretty stable and simple, so I'm not too concerned about having to maintain two clients until the full API has been implemented in the new convention.
| from .client import ClientFeature | ||
|
|
||
| class TokenRoles(ClientFeature): | ||
| def write(self, role, allowed_policies=None, orphan=None, period=None, |
There was a problem hiding this comment.
Could we standardize on "create" instead of write?
|
|
||
| return self._post('/v1/auth/token/roles/{0}'.format(role), json=params) | ||
|
|
||
| def read(self, role): |
There was a problem hiding this comment.
Could we standardize on "get" instead of read?
|
Ah! 🤦 There was a comment in the original issue about semantic versioning that makes a lot more sense now... I should have some spare time over the weekend to move this to |
|
I think I like where this is going.... Will look at some other PR's first, but as HCV adds more features this kind of structure will be key to grow |
|
If we get this merged I'd be happy to start working on some more of the existing features! Keep in mind that it's Hacktoberfest so I've plenty of incentive to work toward feature parity with v1 for the rest of the month. |
|
Hey @otakup0pe @ianunruh, Have you guys got any further feedback for this? |
|
I think we can merge this. How do we want to handle not maintaining duplicates of the functions? Maybe have the old v1 stuff call into the v2? Save that problem for when there is more api coverage? |
|
@otakup0pe I think @ianunruh wanted to maintain the two versions until v2 was ready. I like the idea of v1 functions calling into v2 but you might be right about holding off till we have more coverage. We want to be sure v2 doesn't introduce regressions before subjecting v1 to it without their cooperation. The only way we can be sure v2 is ready is if we have enough people using it first. What you think? |
|
My two cents are that test coverage should be good enough to ensure no obvious regressions are introduced. I'm not a big fan of versioning a Python module interface as proposed here. Thats the point of the version on the package / module itself. So I would vote either for a v1.0.0 module branch that is worked on in parallel, or better yet, do the "strangler" pattern sort of idea and have the v1 functions call in to the newer code as alluded to. With all that said, I'm a newcomer to this module in particular so I may be missing some historical context. In any case, did you still want to pursue getting this PR merged in @nicr9? I'm trying to get some of the outstanding PRs and issues cleaned up here... |
|
I would still like to see us go down this route, but in the interest of keeping the open PR list here tidy, I'm going to close out this particular PR for now. If no one else picks this sort of refactor back up in the near term, I'll try to incorporate these type of changes into a new PR. |
I wanted to get a head start on this while the token roles stuff was fresh in my mind!
I've added two new classes: ClientFeature and TokenRoles. The former is a wrapper around the client for the basic API calls (post, read, list and delete). The later inherits the former and is where the new token roles functionality lives.
Also added further checks to current tests.
Motivation for changes is as discussed in #95.
BREAKINGV1 -> V2 CHANGES: