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

APPS/{x509,req}: Fix description and diagnostics of -key, -in, etc. options #16440

Closed
wants to merge 2 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Aug 27, 2021

Over the years, the x509 and req app options have become a mess
and their documentation and help output is partly outdated, incomplete, or even wrong.
I recently came across this issue again when extending the tests in #16342.

This PR improves the description of the -key, -in, and related options
and adds some warnings for useless or problematic option combinations.

Note that this does not change the semantics of the options.
It would be good to make them more consistent or at least flag some combinations as error,
but we likely cannot do this before the 3.0 release.

  • documentation is added or updated

@DDvO DDvO added approval: otc review pending This pull request needs review by an OTC member triaged: documentation The issue/pr deals with documentation (errors) labels Aug 27, 2021
@DDvO DDvO added this to the 3.0.0 milestone Aug 27, 2021
@t8m t8m removed this from the 3.0.0 milestone Sep 7, 2021
@t8m
Copy link
Member

t8m commented Sep 7, 2021

OTC: not blocking 3.0.0 release. OTC will decide later if this is acceptable for 3.0 branch.

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Sep 8, 2021
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 9, 2021
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@DDvO
Copy link
Contributor Author

DDvO commented Sep 10, 2021

Can one meanwhile merge to master again as usual?

@paulidale
Copy link
Contributor

Yes, you can merge to master. We're still a while from determining futures though but a change like this seems reasonable to include now.

openssl-machine pushed a commit that referenced this pull request Sep 11, 2021
…ptions

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16440)
openssl-machine pushed a commit that referenced this pull request Sep 11, 2021
…y option

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #16440)
@DDvO
Copy link
Contributor Author

DDvO commented Sep 11, 2021

Merged - thanks @paulidale

@DDvO DDvO closed this Sep 11, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Sep 13, 2021

Yes, you can merge to master. We're still a while from determining futures though but a change like this seems reasonable to include now.

Is it already clear if/how to potentially (back-)port this to 3.0?
Or are documentation improvements not considered important enough for that?
Re-opening this for now, just as a reminder until this is clarified.

@DDvO DDvO reopened this Sep 13, 2021
@t8m
Copy link
Member

t8m commented Sep 13, 2021

It is not a pure documentation fix. It would be probably better to open a new PR against 3.0 branch if you want OTC to consider this.

@paulidale paulidale added the hold: need otc decision The OTC needs to make a decision label Sep 14, 2021
@paulidale
Copy link
Contributor

I've flagged this for OTC discussion with respects to merging all of this or just the documentation changes to the 3.0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge hold: need otc decision The OTC needs to make a decision triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants