adding batch_input to transit.sign_data #988

[deidax]

Resolves #988 by adding the batch_input to the transit sign_data method and updating the params.

Resolve the issue #989 by checking if the value of hash_algorithm is None hash_algorithm = '' if not hash_algorithm else hash_algorithm and updating the api_path "/v1/{mount_point}/sign/{name}/{hash_algorithm}"

[briantist]

thanks for submitting this! I've left a few small comments, but I still need to look at the underlying issues more deeply. I'm a little overloaded at the moment, but I will get back to it at some point.

[codecov]

Codecov Report

Merging #990 (6c74534) into main (2786bb8) will increase coverage by 0.10%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #990      +/-   ##
==========================================
+ Coverage   81.78%   81.89%   +0.10%     
==========================================
  Files          65       65              
  Lines        2992     3032      +40     
==========================================
+ Hits         2447     2483      +36     
- Misses        545      549       +4     

Impacted Files	Coverage Δ
hvac/api/secrets_engines/transit.py	88.43% <33.33%> (-2.35%)	⬇️

... and 7 files with indirect coverage changes

[briantist]

Please ensure you run black and flake8 before pushing commits to avoid lint failures. If you haven't seen it there's some info in the contributing guide on setting up the local poetry environment:

• https://hvac.readthedocs.io/en/stable/contributing.html

Then you can run:

poetry run black .
poetry run flake8 .

[briantist]

Hello @deidax !

The hvac project is simplifying its developer workflow and removing the develop branch. Please be aware for future PRs that the project's default branch is now main.

Since your PR was opened before this change, I have:

• updated your PR to merge into the project's main branch
• rebased your branch against main and force-pushed it (please pull before making any further changes)

I will come back to review the actual content as soon as I can, thanks for your patience!

[briantist]

Thanks for submitting this! As commented below, I'd like to get more information on the hash_algorithm issue.

For batch_input, it needs docs, and I'd really like to see the tests updated for this, is that something you could take a look at?

[deidax]

Thanks for submitting this! As commented below, I'd like to get more information on the hash_algorithm issue.

For batch_input, it needs docs, and I'd really like to see the tests updated for this, is that something you could take a look at?

@briantist Before running any tests, please consider the following: According to the vault documentation for the transit.sign_data function, if batch_input is set, hash_input should be ignored. Currently, the function does not handle this case properly, as hash_input is a required parameter. To use batch_input instead, the user should set hash_input=None.

[briantist]

@deidax could you please revert the merge of the develop branch into the PR? Both of these commits should be reverted or removed via interactive rebase:

• 38378d6
• eca6563

As well as the 4 commits shown here that have only me as the author:

• https://github.com/hvac/hvac/pull/990/commits

See here for why: #990 (comment)
If you need help, I can try to do this and force push your branch.

Yes I would appreciate it if you can do it

Sure, I'm going to attempt this now. Since this operation can lose data, you may want to create a new branch off of your current one locally in case you need to recover commits later.

@deidax I believe this was successful! I also pushed an additional commit where I ran black to fix the formatting/lint error.

For this PR, please do not use the GitHub UI "sync fork" button. You can ignore the message telling you that your branch is behind.

What's happening is that you have a develop branch which tracks our develop branch, but we're not using that one anymore. I previously rebased your develop branch to actually be based on our main branch.

I think this will not be a problem for your future PRs (I hope there will be more 😁), which should be based off the main branch to begin with.

I'm very sorry for the confusion about this, I know it's not a good experience especially for a first-time contributor to a project, it was just bad timing on my part.

[briantist]

Just some wording and formatting suggestions

[briantist]

a few more docs re-ordering, I'll go ahead and commit these, and if you want to change anything with the edits, feel free to push more commits

[briantist]

This looks good to me! Thanks so much for submitting and sticking through the edits and suggestions.

Since I was the last committer, I'll wait for you to look at those changes before merging. Let me know if it all looks good to you.

[deidax]

This looks good to me! Thanks so much for submitting and sticking through the edits and suggestions.

Since I was the last committer, I'll wait for you to look at those changes before merging. Let me know if it all looks good to you.

@briantist yeah it looks good to me. thank you