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

Fix: Remove Batch from API options, set to true by default #1208

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

reglim
Copy link
Contributor

@reglim reglim commented Aug 17, 2023

It doesn't make sense to expose the Batch option, which, if false enables input from stdin for gpg passphrases, which blocks the api thread. Because of this, I removed the option, and set it to true by default.

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

  • Remove Batch option
  • Set Batch to true internally

Why this change is important?

So the API can't get hung up because a user tries to sign a publish with a key that requires a passphrase he didn't provide in the request.

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@reglim reglim force-pushed the fix/1106-useless-batch-option-for-api branch 7 times, most recently from 949df70 to 8be8a47 Compare August 18, 2023 07:28
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #1208 (8be8a47) into master (1df8cff) will decrease coverage by 0.03%.
Report is 4 commits behind head on master.
The diff coverage is 44.44%.

❗ Current head 8be8a47 differs from pull request most recent head 519747a. Consider uploading reports for the commit 519747a to get more accurate results

@@            Coverage Diff             @@
##           master    #1208      +/-   ##
==========================================
- Coverage   65.99%   65.97%   -0.03%     
==========================================
  Files         143      143              
  Lines       16196    16192       -4     
==========================================
- Hits        10688    10682       -6     
- Misses       4754     4758       +4     
+ Partials      754      752       -2     
Files Changed Coverage Δ
api/publish.go 0.00% <0.00%> (ø)
deb/graph.go 0.00% <0.00%> (ø)
pgp/internal.go 57.86% <ø> (ø)
api/task.go 42.15% <100.00%> (ø)
deb/import.go 76.50% <100.00%> (+0.92%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@reglim reglim force-pushed the fix/1106-useless-batch-option-for-api branch from 8be8a47 to 519747a Compare August 18, 2023 11:32
@reglim reglim changed the title Draft: Improvement: Add Test for publish with passphrase but batch=false Improvement: Add Test for publish with passphrase but batch=false Aug 18, 2023
@reglim reglim changed the title Improvement: Add Test for publish with passphrase but batch=false Fix: Remove Batch from API options, set to true by default Aug 18, 2023
Copy link
Contributor

@randombenj randombenj left a comment

Choose a reason for hiding this comment

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

LGTM

@reglim reglim merged commit 40c242f into master Sep 14, 2023
4 of 5 checks passed
@reglim reglim deleted the fix/1106-useless-batch-option-for-api branch September 14, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants