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

Integration tests for certificate policy manager #753

Merged
merged 8 commits into from
Aug 2, 2023

Conversation

GabrielNagy
Copy link
Contributor

Best to review this commit by commit to avoid being overwhelmed by the golden file updates.

Fixes UDENG-1059

Instead of having defaults for all needed directories in the certificate
Python script, only make the state directory overridable since the
others are descendants of it.

This will ease integration testing as we will only need to expose the
state directory and the global trust directory.
To aid in integration tests, these must be editable via the
configuration file.
To be able to use the samba mock in the integration tests we need to
append to the PYTHONPATH variable instead of overriding it. This way
previously set paths will take precedence.
Removing a directory with `os.removedirs` will fail if it's non-empty.
We can circumvent this behavior by using `shutil.rmtree` instead.
Old data - policy configured & disabled
Up to date - policy configured & enabled

Some things to notice in the tests behavior:
- no changes to user golden files with no initial state
- purging machine policies removes the samba directory if it exists

Fixes UDENG-1059
@GabrielNagy GabrielNagy marked this pull request as ready for review August 1, 2023 10:07
@GabrielNagy GabrielNagy requested a review from a team as a code owner August 1, 2023 10:07
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Looks great to me!

I do have one comment though (and I'm sorry for not spotting it in the other PR, I noticed it now when looking at the golden files)... Why is the unit test for the certificates manager called TestPolicyApply instead of TestApplyPolicy (which is the actual function name and the pattern that we use)?

@GabrielNagy
Copy link
Contributor Author

Mm good catch, I assumed I copied that from a different test file but it seems to be my original creation 😅 - I'll amend the test name.

Update the certificate test name to match the tested method name (and
our convention).
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Nothing to add, excellent work! Yeah, it was easier commit per commit.

TBH, I trust you on the .pol files edit :)

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Nicely done!

@GabrielNagy GabrielNagy merged commit 50589d9 into main Aug 2, 2023
4 checks passed
@GabrielNagy GabrielNagy deleted the certificate-integration-tests branch August 2, 2023 11:29
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.

3 participants