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

Rabbitmq health checks for Bytes integration tests instead of sleep() #920

Merged
merged 4 commits into from
May 4, 2023

Conversation

Donnype
Copy link
Contributor

@Donnype Donnype commented May 4, 2023

Changes

Attempts to tackle the flaky issues from the Bytes CI. Also see this action for the logs: https://github.com/minvws/nl-kat-coordination/actions/runs/4883917359/jobs/8716061057?pr=761


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified;
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

ammar92
ammar92 previously approved these changes May 4, 2023
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Not entirely sure if this is exactly the bottleneck (since we don't have the facts), but it does look very plausible and it's good to have these health checks anyway.

The only thing is we should probably have fewer retries than you suggested since that service shouldn't take that long to load

@Donnype Donnype marked this pull request as ready for review May 4, 2023 14:57
@Donnype Donnype requested a review from a team as a code owner May 4, 2023 14:57
@Donnype
Copy link
Contributor Author

Donnype commented May 4, 2023

@ammar92 check!

@dekkers
Copy link
Contributor

dekkers commented May 4, 2023

Not entirely sure if this is exactly the bottleneck (since we don't have the facts), but it does look very plausible and it's good to have these health checks anyway.

I remember seeing an amqp error in the tests, so this might be the cause...

@dekkers dekkers closed this May 4, 2023
@dekkers dekkers reopened this May 4, 2023
@dekkers dekkers merged commit 1041b5f into main May 4, 2023
@dekkers dekkers deleted the fix/bytes-flaky-rabbitmq-issues branch May 4, 2023 15:49
jpbruinsslot added a commit that referenced this pull request May 8, 2023
* main: (39 commits)
  Add delete oois permission (#783)
  Implement openmetrics endpoint in mula (#662)
  Rabbitmq health checks for Bytes integration tests instead of sleep() (#920)
  Groups and Permissions for Organization member (#606)
  Wrap all import errors in ModuleExceptions while resolving plugins (#818)
  Feature/octopoes path queries (#851)
  Fix reset scan profile bug and reduce octopoes calls in view (#910)
  Fix fierce normalizer to create hostnames without a trailing dot (#909)
  Updated playwright (#903)
  Use superuser credentials from .env file (#860)
  Localize account scoped url's (#861)
  Dependabot merges (#902)
  Updated linter configs (#834)
  Fix cancel url at organization edit view (#850)
  Add OpenTelemetry tracing to Mula, Bytes, Keiko, and KATalogus (#831)
  Normalizers should be defined as generators (#830)
  Rdo workflows (#816)
  Updated linter configs (#825)
  Add `rocky_version` to footer (#821)
  Update PR template with PGP-sign checkbox and absolute URL's (#826)
  ...
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