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

ipatests: Tests for ipahealthcheck tool with IPA external CA #4883

Closed
wants to merge 2 commits into from
Closed

ipatests: Tests for ipahealthcheck tool with IPA external CA #4883

wants to merge 2 commits into from

Conversation

menonsudhir
Copy link
Contributor

This testsuite checks whether the healthcheck tool reports correct status in a scenario when IPA server is setup with
external self-signed CA. Below are the checks covered.

IPACRLManagerCheck
IPACertmongerCA
IPAOpenSSLChainValidation
IPANSSChainValidation
IPARAAgent

mrizwan93
mrizwan93 requested changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@mrizwan93 mrizwan93 left a comment

Choose a reason for hiding this comment

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

Hello @menonsudhir
Please find inline comments.

ipatests/test_integration/test_ipahealthcheck.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ipahealthcheck.py Show resolved Hide resolved
ipatests/test_integration/test_ipahealthcheck.py Outdated Show resolved Hide resolved
ipatests/test_integration/test_ipahealthcheck.py Outdated Show resolved Hide resolved
@menonsudhir menonsudhir force-pushed the external_ca branch 3 times, most recently from 64f3cc6 to bdba45a Compare July 3, 2020 08:47
@menonsudhir
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@menonsudhir menonsudhir requested a review from rcritten July 3, 2020 09:18
@menonsudhir menonsudhir added the ipa-4-8 Mark for backport to ipa 4.8 label Jul 3, 2020
@menonsudhir
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@menonsudhir menonsudhir added re-run Trigger a new run of PR-CI and removed re-run Trigger a new run of PR-CI labels Jul 6, 2020
@menonsudhir menonsudhir force-pushed the external_ca branch 6 times, most recently from a39dd70 to fad18dd Compare July 20, 2020 09:52
flo-renaud
flo-renaud reviewed Aug 12, 2020
View reviewed changes
Copy link
Contributor

@flo-renaud flo-renaud left a comment

Choose a reason for hiding this comment

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

Hi @menonsudhir
the test was added to the nightly definitions in the temp commit, please move the diff to another commit that won't be removed.
I also noticed one comment from Rob that wasn't addressed:

I think this test should install a replica as well to ensure that the crlmanagercheck returns crlgen_enabled False.

@menonsudhir menonsudhir force-pushed the external_ca branch 2 times, most recently from b03cbd6 to e1df335 Compare August 12, 2020 09:16
@menonsudhir
Copy link
Contributor Author

Hi @menonsudhir
the test was added to the nightly definitions in the temp commit, please move the diff to another commit that won't be removed.
I also noticed one comment from Rob that wasn't addressed:

I think this test should install a replica as well to ensure that the crlmanagercheck returns crlgen_enabled False.

Hi @menonsudhir
the test was added to the nightly definitions in the temp commit, please move the diff to another commit that won't be removed.
I also noticed one comment from Rob that wasn't addressed:

I think this test should install a replica as well to ensure that the crlmanagercheck returns crlgen_enabled False.
@flo-renaud , i have added the code back which was removed while rebase was done.

@flo-renaud
Copy link
Contributor

@menonsudhir
pycodestyle reported an error:

-----------
./ipatests/test_integration/test_ipahealthcheck.py:1654:1: W293 blank line contains whitespace

@menonsudhir
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rcritten
Copy link
Contributor

This looks ok to me with one minor thing. I think the timeout for the nightly tests will need to be bumped up to accommodate the new test class, maybe try 7200. IMHO this can be done at the same time that the temp commit is dropped to avoid another run cycle.

@flo-renaud do you agree?

@menonsudhir
ipatests: Tests for ipahealthcheck tool with IPA external
d26ef5c 
This testsuite checks whether the healthcheck tool reports
correct status in a scenario when IPA server is setup with
external self-signed CA. Below are the checks covered

IPACRLManagerCheck
IPACertmongerCA
IPAOpenSSLChainValidation
IPANSSChainValidation
IPARAAgent
@menonsudhir
Modified nightly YAML files to include ipa-healthcheck
e92505c 
ExternalCA Tests
@flo-renaud
Copy link
Contributor

This looks ok to me with one minor thing. I think the timeout for the nightly tests will need to be bumped up to accommodate the new test class, maybe try 7200. IMHO this can be done at the same time that the temp commit is dropped to avoid another run cycle.

@flo-renaud do you agree?

Yes, the new test takes ~30min and test-Ipahealthcheck used to need 1h, so 7200 should be OK.

@menonsudhir menonsudhir added the re-run Trigger a new run of PR-CI label Aug 13, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Aug 13, 2020
@menonsudhir
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@menonsudhir
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@menonsudhir menonsudhir added ipa-4-8 Mark for backport to ipa 4.8 and removed ipa-4-8 Mark for backport to ipa 4.8 labels Aug 17, 2020
@flo-renaud
Copy link
Contributor

Tests are green, all comments were addressed, ACK.

@flo-renaud flo-renaud added ack Pull Request approved, can be merged pushed Pull Request has already been pushed and removed needs rebase Pull Request cannot be automatically merged - needs to be rebased labels Aug 17, 2020
@flo-renaud
Copy link
Contributor

master:

  • 400ef3a ipatests: Tests for ipahealthcheck tool with IPA external
  • 7642ce3 Modified nightly YAML files to include ipa-healthcheck ExternalCA Tests

@flo-renaud flo-renaud closed this Aug 17, 2020
@flo-renaud
Copy link
Contributor

@menonsudhir
A manual backport is required for ipa-4-8 branch, can you do it? thanks

@menonsudhir
Copy link
Contributor Author

@menonsudhir
A manual backport is required for ipa-4-8 branch, can you do it? thanks

@flo-renaud have raised a manual backport.
#5034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcritten rcritten rcritten left review comments

@flo-renaud flo-renaud flo-renaud left review comments

@mrizwan93 mrizwan93 mrizwan93 requested changes

Assignees

@rcritten rcritten

Labels
ack Pull Request approved, can be merged ipa-4-8 Mark for backport to ipa 4.8 pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
5 participants
@menonsudhir @rcritten @mrizwan93 @flo-renaud @freeipa-pr-ci