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.

Sign up for GitHub

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

Jump to bottom

Per other commands, make progress dots in req only w/ -verbose #21937

Closed
wants to merge 1 commit into from
Closed

Per other commands, make progress dots in req only w/ -verbose #21937

wants to merge 1 commit into from

Conversation

pprindeville
Copy link
Contributor

Checklist
  • documentation is added or updated

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Sep 4, 2023
@t8m
Copy link
Member

t8m commented Sep 4, 2023

AFAIK for the keygen commands we did not change the default - i.e. by default the progress is still reported there.

@t8m
Copy link
Member

t8m commented Sep 4, 2023

But I do not think we want to change the default for verbose to be 1 here as that has further implications. IMO this should use some additional variable such as progress - and that would be 1 by default, set to 0 by quiet and set to 1 by verbose.

@pprindeville
Copy link
Contributor Author

But I do not think we want to change the default for verbose to be 1 here as that has further implications. IMO this should use some additional variable such as progress - and that would be 1 by default, set to 0 by quiet and set to 1 by verbose.

Done

t8m
t8m reviewed Sep 4, 2023
View reviewed changes
doc/man1/openssl-req.pod.in Outdated Show resolved Hide resolved
@pprindeville
Per other commands, make progress dots in req only w/ -verbose
b40b531 
Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>
@t8m t8m added approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing labels Sep 4, 2023
beldmit
beldmit approved these changes Sep 4, 2023
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit
Copy link
Member

beldmit commented Sep 4, 2023

The test failure seems irrelevant

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 4, 2023
@paulidale
Copy link
Contributor

But I do not think we want to change the default for verbose to be 1 here as that has further implications. IMO this should use some additional variable such as progress - and that would be 1 by default, set to 0 by quiet and set to 1 by verbose.

We really should make an effort to get consistency across the commands. Possibly for version 4.

@pprindeville
Copy link
Contributor Author

I think that's the last of the progress-emitting commands... but yeah, it wouldn't be bad to make progress dots turned off by default for 4.0.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor

Merged, thanks for the contribution.

@paulidale paulidale closed this Sep 5, 2023
openssl-machine pushed a commit that referenced this pull request Sep 5, 2023
@pprindeville @paulidale
Per other commands, make progress dots in req only w/ -verbose
c1673a6 
Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21937)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pprindeville @t8m @beldmit @paulidale @openssl-machine