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

Add GCP Auth Method Class #240

Merged
merged 27 commits into from Oct 24, 2018

Conversation

@seanmalloy
Copy link
Contributor

commented Aug 1, 2018

No unit or integration tests yet. Code
has not been tested yet.

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

Hullo again @seanmalloy! I am happy to see some more GCP-related API routes getting coverage here. :D

So in #242 I'm hoping to establish a different pattern for how auth (and ultimately secret methods) are organized. Given that, and assuming the new pattern being proposed doesn't raise any general concerns on your part, I'd love to see these GCP method additions follow that same sort of pattern. I.e., instead of adding new methods to the quite large Client class, we could add a hvac.api.auth.gcp.Gcp class to hold these methods. Since this is a new development without corresponding updates to our contributing guidelines, I figure we could either:

  • Evaluate this PR's changes as-is once test coverage has been included. Assuming everything checks out, we could merge the changes as they stand and I would then be happy to refactor things before the next release. Since I'm hoping to reduce the amount of code in the Client class, I wouldn't want to cut a release including these methods in that class. (Once folks start calling the methods in one location, its tricky to migrate them to a different source file without risking breaking external usage relying on that status quo.)
  • If you're down to follow the same sort of pattern being included with #242 and update these changes to match, I'd be happy to help provide guidance or otherwise provide any clarification should anything be unclear.

@jeffwecan jeffwecan added this to the 0.6.4 milestone Aug 2, 2018

@seanmalloy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

@jeffwecan sounds good. I can change the GCP auth code to use your new pattern. I should be able to start working on this PR again later this week.

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@seanmalloy 👍. Feel free to pull in all the recent GCP additions to a new class or just the bits you're looking to add here. Either is fine by me.

For what it is worth, I've been using a small helper script to scrape Vault API docs (e.g., https://www.vaultproject.io/api/auth/gcp/index.html) and stub out classes based on that content. So if it is helpful, here is a rough pass of an autogenerated Gcp class: https://github.com/hvac/hvac/compare/master...jeffwecan:gcp_example?expand=1

@codecov-io

This comment has been minimized.

Copy link

commented Aug 16, 2018

Codecov Report

Merging #240 into master will increase coverage by 0.86%.
The diff coverage is 96.26%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   91.11%   91.98%   +0.86%     
==========================================
  Files          20       22       +2     
  Lines        1306     1410     +104     
==========================================
+ Hits         1190     1297     +107     
+ Misses        116      113       -3
Impacted Files Coverage Δ
hvac/api/__init__.py 100% <100%> (ø) ⬆️
hvac/api/gcp.py 100% <100%> (ø)
hvac/api/auth/__init__.py 100% <100%> (ø) ⬆️
hvac/v1/__init__.py 86.41% <100%> (+0.03%) ⬆️
hvac/utils.py 89.58% <100%> (+21.16%) ⬆️
hvac/api/auth/gcp.py 94.59% <94.59%> (ø)
@seanmalloy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

Count down to ready for review ...

  • configure method works
  • configure tests written
  • read_config method works
  • read_config tests written
  • delete_config method works
  • delete_config tests written
  • create_role method works
  • create_role tests written
  • edit_service_accounts_on_iam_role method works
  • edit_service_accounts_on_iam_role tests written
  • edit_service_accounts_on_iam_role method works
  • edit_service_accounts_on_iam_role tests written
  • edit_labels_on_gce_role method works
  • edit_labels_on_gce_role tests written
  • read_role method works
  • read_role tests written
  • list_roles method works
  • list_roles tests written
  • delete_role method works
  • delete_role tests written
  • login method works
  • login tests written
  • deprecated login method works
  • deprecated login tests written

@jeffwecan jeffwecan modified the milestones: 0.6.4, 0.6.5 Aug 31, 2018

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Sep 29, 2018

Hey @seanmalloy, Since we're coming up on the end of the month / about when I wanted to cut the 0.6.5 hvac release, I may try to get the work you've done already over the finish line and push up some commits to your branch if you don't mind. So if I don't hear back with any objections by next Monday, I'll probably go that route 👍

jeffwecan added some commits Oct 2, 2018

jeffwecan added some commits Oct 3, 2018

@jeffwecan jeffwecan changed the title WIP: add more GCP auth backend methods Add GCP Auth Method Class Oct 3, 2018

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2018

@seanmalloy: I tried to get these additions mostly filled out. Unless you see any issues with the commits I tacked on, I'll get this merged in for the next hvac release over the next day or so.

@jeffwecan jeffwecan removed the enhancement label Oct 3, 2018

jeffwecan added some commits Oct 19, 2018

@jeffwecan jeffwecan merged commit a70cc2d into hvac:master Oct 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Breakout Client Class Methods automation moved this from In progress to Done Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.