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

Re-add arguments to create_or_update_role() from old API #842

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Jul 28, 2022

The previous, deprecated create_token_role() method has arguments period and explicit_max_ttl that were dropped in the new one for some reason (in #734).

Re-added these two arguments to the new create_or_update_role() method (using new spelling token_period, token_explicit_max_ttl).

There are plenty more that are supported by Vault but missing here: https://www.vaultproject.io/api-docs/auth/token#create-update-token-role

@intgr intgr requested a review from a team as a code owner July 28, 2022 14:58
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #842 (a216803) into main (936e3ef) will decrease coverage by 2.71%.
The diff coverage is n/a.

❗ Current head a216803 differs from pull request most recent head 9ed04ec. Consider uploading reports for the commit 9ed04ec to get more accurate results

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
- Coverage   84.78%   82.08%   -2.71%     
==========================================
  Files          65       65              
  Lines        3083     3019      -64     
==========================================
- Hits         2614     2478     -136     
- Misses        469      541      +72     
Impacted Files Coverage Δ
hvac/api/auth_methods/token.py 83.33% <ø> (ø)

... and 12 files with indirect coverage changes

@briantist briantist added enhancement a new feature or addition token Related to the token auth method labels Jan 28, 2023
@briantist briantist force-pushed the develop branch 2 times, most recently from 085b858 to 9dbfa98 Compare May 10, 2023 06:41
@briantist briantist changed the base branch from develop to main June 17, 2023 21:52
@briantist briantist force-pushed the readd-create_or_update_role-args branch from fa3da73 to a216803 Compare June 17, 2023 21:53
@briantist

This comment was marked as outdated.

Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

Hi @intgr , thanks for submitting this! Would you be able to take a look at the tests for token auth as well, and add some coverage for these parameters?

hvac/api/auth_methods/token.py Show resolved Hide resolved
hvac/api/auth_methods/token.py Show resolved Hide resolved
@briantist briantist added the auth methods generally related to a Vault auth method label Jun 24, 2023
@briantist briantist self-assigned this Jun 24, 2023
intgr added 2 commits July 9, 2023 19:17
The previous, deprecated `create_token_role()` method has arguments `period` and `explicit_max_ttl` that were dropped in the new one for some reason.
@briantist briantist force-pushed the readd-create_or_update_role-args branch from a216803 to 715f6b7 Compare July 9, 2023 23:17
@briantist briantist added this to the 1.2.0 milestone Jul 10, 2023
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

@intgr thanks again for this PR, I've gone ahead an switched the parameter order and added some unit tests for token auth after rebasing your branch. Cheers!

@briantist briantist merged commit b52ed19 into hvac:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth methods generally related to a Vault auth method enhancement a new feature or addition token Related to the token auth method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants