Skip to content

flakiness - fix cert rotation tests #19

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

Merged
merged 4 commits into from
Apr 23, 2025
Merged

Conversation

nammn
Copy link
Collaborator

@nammn nammn commented Apr 22, 2025

Summary

Proof of Work

Checklist

  • Have you linked a jira ticket and/or is the ticket in the title?
  • Have you checked whether your jira ticket required DOCSP changes?
  • Have you checked for release_note changes?

Reminder (Please remove this when merging)

  • Please try to Approve or Reject Changes the PR, keep PRs in review as short as possible
  • Our Short Guide for PRs: Link
  • Remember the following Communication Standards - use comment prefixes for clarity:
    • blocking: Must be addressed before approval.
    • follow-up: Can be addressed in a later PR or ticket.
    • q: Clarifying question.
    • nit: Non-blocking suggestions.
    • note: Side-note, non-actionable. Example: Praise
    • --> no prefix is considered a question

@nammn nammn requested review from lucian-tosa and m1kola April 22, 2025 14:39
Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Looks good overall. My comments are mostly questions. There is only one suggestion to change assert_certificate_rotation slightly.

timeout=timeout,
)

from kubetester.mongodb import Phase
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking q: any reason not to put this to the top of the file with rest of the imports?

Comment on lines +903 to +904
old_ac_version = KubernetesTester.get_automation_config()["version"]
rotate_cert(namespace, certificate_name, should_block_until_ready=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert_certificate_rotation gives an impression that it does not make any changes and only asserts when in fact it does.

What do you think about the following suggestion?

  1. Moving these two lines into the caller functions
  2. Make assert_certificate_rotation accept old_ac_version as an argument

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed! bf2d183

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit of context for transparency. We discussed it in DM: while I think it is good to keep mutation (cert rotation) separate from assertions, @nammn wanted to to keep everything together to avoid forgetting to do part of the checks.

At the same time I think it is more important to address the flake than worry about semantics here. We landed on the middle ground: renaming the function to rotate_and_assert_certificates to more clearly indicate the purpose.

@m1kola m1kola mentioned this pull request Apr 23, 2025
3 tasks
@nammn nammn requested a review from m1kola April 23, 2025 09:23
@nammn nammn merged commit 41454fa into master Apr 23, 2025
7 of 8 checks passed
@m1kola m1kola mentioned this pull request Apr 23, 2025
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.

2 participants