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

Fix rabbitmq test #232

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Fix rabbitmq test #232

merged 1 commit into from
Mar 3, 2025

Conversation

vyzigold
Copy link
Contributor

@vyzigold vyzigold commented Mar 3, 2025

No description provided.

@@ -15,10 +15,10 @@
- kind: ceilometer
name: ceilometer
condition_type: Ready
- kind: rabbitmq
- kind: rabbitmqs.rabbitmq.openstack.org
Copy link

Choose a reason for hiding this comment

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

While this is good and it will fix main, I'm wondering about the implications of this change wrt previous releases.
I see we don't follow a FRx branching strategy, which I think it makes sense for this project, but because we don't have rabbitmqs in the previous releases (e.g. FR1), I'm not sure we should improve this code to check both CRDs and see which one has a Rabbit object.
One idea might be to simply extend this check to support both CRDs, unless we don't care about FR1 anymore as long as we branch FR2.
@jlarriba what do you think? This change is sufficient to fix main and test FR2 (that is going to have a branch today), but it might break FR1 because kind: rabbitmqs.rabbitmq.openstack.org isn't there yet. How do we deal with this kind of interface changes?

Copy link

Choose a reason for hiding this comment

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

both versions have rabbitmqclusters, which is the actual object handling rabbitmq deployment. could just check for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you're right Francesco, we probably want to be able to test both for now. I'll rewrite this to use rabbitmqclusters as Martin suggested. This should get the tests into a working state for now and we can figure out what we want to do in the future later.

Copy link

Choose a reason for hiding this comment

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

right, thank you both for the help with this. I didn't have a fresh environment to check my suggestions/assumptions and I think it's better to fallback to rabbitmqclusters if it resolves the group we're checking. It works for previous releases and we should be able to solve this problem for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its my understanding that there will be no more patches into FR1 as soon as FR2 is released, right? So that is no concern.

@vyzigold vyzigold force-pushed the jwysogla-fix-rabbitmq branch from 1a5aa2f to 291f499 Compare March 3, 2025 09:37
@jlarriba jlarriba merged commit 77d6a22 into master Mar 3, 2025
8 checks passed
@jlarriba jlarriba deleted the jwysogla-fix-rabbitmq branch March 3, 2025 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants