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

include db engine as implemented class #455

Merged
merged 1 commit into from May 30, 2019

Conversation

3 participants
@drewmullen
Copy link
Contributor

commented May 29, 2019

database secrets engine wasnt enabled though the code was included. probably a good example for why we need unit tests :)

also - @jeffwecan may want to investigate why the error said '"%s" auth method class instead of database secret engine class

fix for #454

@codecov-io

This comment has been minimized.

Copy link

commented May 29, 2019

Codecov Report

Merging #455 into develop will increase coverage by 0.53%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           develop   #455      +/-   ##
=========================================
+ Coverage    87.47%    88%   +0.53%     
=========================================
  Files           50     50              
  Lines         2658   2659       +1     
=========================================
+ Hits          2325   2340      +15     
+ Misses         333    319      -14
Impacted Files Coverage Δ
hvac/api/secrets_engines/__init__.py 100% <100%> (ø) ⬆️
hvac/api/secrets_engines/database.py 35.89% <0%> (+35.89%) ⬆️
@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

Ah good catch! My bad for missing that bit in the initial PR (the fact it was easy enough to miss is more fodder for #434 imho ;) ). The '"%s" auth method class is just a copy/paste carry over that should have been updated to state secrets engine class. If you wanna grab that fix here as well I would be down!

@jeffwecan jeffwecan added this to the 0.9.2 milestone May 30, 2019

@jeffwecan jeffwecan added the database label May 30, 2019

@drewmullen

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@jeffwecan so if i changed:

raise NotImplementedError('"%s" auth method class not currently implemented.')

wouldnt that also change the error when its searching for all 3 implementation lists ('AuthMethods', 'SecretsEngines', 'SystemBackend',)? i was trying to figure that out yesterday

the change might be more significant but im still pretty new to inheritance, etc

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

Ah yeah okay, didn't realize that bit was on the base class. We can skip it for now if you want and address that in another PR if you'd like in that case.

@drewmullen

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

yeah that sounds good. im gonna add a couple more bits to the secrets.database class too. will try to get that in by the weekend

@jeffwecan jeffwecan merged commit e8ae13a into hvac:develop May 30, 2019

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 May 30, 2019

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.