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

test_csrgen: adjusted comparison test scripts for CSRGenerator #537

Closed
wants to merge 1 commit into from

Conversation

Rezney
Copy link
Collaborator

@Rezney Rezney commented Mar 3, 2017

Commit ada91c2 introduced changes in "csrgen/templates/openssl_base.tmpl"
which broke the following 2 tests:

test_CSRGenerator.test_userCert_OpenSSL
test_CSRGenerator.test_caIPAserviceCert_OpenSSL

The tests use files caIPAserviceCert_openssl.sh and userCert_openssl.sh
as expected scripts in order to compare scripts generated by CSRGenerator.
E.g. as other parameter was introduced we are now not checking with
"if [[ $# -ne 2 ]]" but rather with if "[[ $# -lt 2 ]]".

https://pagure.io/freeipa/issue/6724

Copy link
Contributor

@apophys apophys left a comment

Choose a reason for hiding this comment

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

Please address the comments inline. Otherwise conditional ACK.

openssl req -new -config "$CONFIG" -out "$CSR" -key $1
rm "$CONFIG"
openssl req -new -config "$CONFIG" -out "$CSR" -key "$KEYFILE" "$@"
rm "$CONFIG"
Copy link
Contributor

@apophys apophys Mar 3, 2017

Choose a reason for hiding this comment

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

The missing newline at the EOF breaks test_CSRGenerator.test_caIPAserviceCert_OpenSSL test case.

Commit ada91c2 introduced changes in "csrgen/templates/openssl_base.tmpl"
which broke the following 2 tests:

    test_CSRGenerator.test_userCert_OpenSSL
    test_CSRGenerator.test_caIPAserviceCert_OpenSSL

The tests use files caIPAserviceCert_openssl.sh and userCert_openssl.sh
as expected scripts in order to compare scripts generated by CSRGenerator.
E.g. as other parameter was introduced we are now not checking with
"if [[ $# -ne 2 ]]" but rather with if "[[ $# -lt 2 ]]".

https://pagure.io/freeipa/issue/6724
@apophys apophys added the ack Pull Request approved, can be merged label Mar 3, 2017
@LiptonB
Copy link
Contributor

LiptonB commented Mar 6, 2017

Thanks for catching this, sorry about the breakage. The change looks good to me.

@martbab
Copy link
Contributor

martbab commented Mar 7, 2017

master:

  • 83e2c2b test_csrgen: adjusted comparison test scripts for CSRGenerator

@martbab martbab added the pushed Pull Request has already been pushed label Mar 7, 2017
@martbab martbab closed this Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants