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

[WIP] Implement Transit engine (#302) #303

Merged
merged 34 commits into from Oct 31, 2018

Conversation

3 participants
@alexandernst
Copy link
Contributor

commented Oct 28, 2018

This is still a WIP. Implements #302

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@jeffwecan I see that there are some transit-related methods in api.v1.__init__, at lines 2267-2699.
What are those? Should I delete them?

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@jeffwecan The code per-se (no unit tests atm) should be ready. Can you check it and verify if everything is ok?

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 28, 2018

Sure, I'll scope it out as I have some more time. I'm also fine with skipping unit tests provided the integration tests cover the code reasonably enough. For the original Client class methods, you can mark them deprecated with a decorator like so: https://github.com/hvac/hvac/blob/master/hvac/v1/__init__.py#L2691-L2696

That way folks have a chance to update their usage before we drop those method entirely.

@jeffwecan jeffwecan added this to the 0.7.0 milestone Oct 28, 2018

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@jeffwecan Do I deprecate them starting from 0.7?

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 28, 2018

Since these additions are intended to go out in 0.7.0, you can mark the old methods with a to_be_removed_in_version of 0.9.0 (ref: https://github.com/hvac/hvac/blob/master/CONTRIBUTING.md#backwards-compatibility-breaking-changes)

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@jeffwecan 👍 Deprecated methods. Ping me if this PR needs more love!

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

Oh, I just saw the CI errors. Do you want me to remove the tests? @jeffwecan

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 28, 2018

Sure! That makes sense to me.

alexandernst added some commits Oct 28, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Oct 28, 2018

Codecov Report

Merging #303 into master will increase coverage by 0.33%.
The diff coverage is 94.3%.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   90.31%   90.64%   +0.33%     
==========================================
  Files          37       39       +2     
  Lines        1735     1893     +158     
==========================================
+ Hits         1567     1716     +149     
- Misses        168      177       +9
Impacted Files Coverage Δ
hvac/api/secrets_engines/__init__.py 100% <100%> (ø) ⬆️
hvac/v1/__init__.py 86.01% <100%> (+0.34%) ⬆️
hvac/constants/transit.py 100% <100%> (ø)
hvac/api/secrets_engines/transit.py 93.23% <93.23%> (ø)
@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@jeffwecan Everything is green now! 👍

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 29, 2018

Thanks @alexandernst! I am about to add in some changes that refactor how auth, secrets, and system backend classes are organized. Once I get those sorted I'll look to merge this in. Should be over the next day or two.

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented Oct 29, 2018

In the meantime, do you mind if I push some changes accommodating that refractor to your branch as needed?

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@jeffwecan Sure, feel free to add whatever you need 👍 😄

jeffwecan added some commits Oct 31, 2018

@jeffwecan jeffwecan force-pushed the Develatio:feature/transit branch from 44a5d90 to 3d34c66 Oct 31, 2018

jeffwecan added some commits Oct 31, 2018

@jeffwecan jeffwecan merged commit 6221123 into hvac:master Oct 31, 2018

1 check passed

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

Ensure Classes for All Auth Methods and Secret Engines automation moved this from In progress to Done Oct 31, 2018

@alexandernst

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

\o/

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