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 OpenShiftAmqpAmqIT by changing service name and bump ActiveMQ Artemis Broker to same as Dev Svcs are using #915

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Oct 17, 2023

Summary

  • service name seems to be changed since 3.5.0.CR1 from amq-broker-amqp to amq-amqp. I didn't find a commit that caused it yet, but I'm clear that it works with 3.4.3 and that oc get service says amq-amqp and this fixes failing OC test
  • we are using old image version, let's use the same version as dev services are using

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik
Copy link
Member Author

run tests

@michalvavrik
Copy link
Member Author

Ad OCP JVM failures - Strimzi failure is known to me and I'm about to work on it tomorrow.

@michalvavrik
Copy link
Member Author

Oracle JVM failure is can't be related.

@michalvavrik
Copy link
Member Author

@mjurc is busy with 3.2, @fedinskiy please have a look when you can

@michalvavrik michalvavrik removed the request for review from mjurc October 18, 2023 07:24
@fedinskiy
Copy link
Contributor

service name seems to be changed

We need to push upstream folks to document this. @michalvavrik , could you look into that?

@fedinskiy
Copy link
Contributor

There is a debug failure as well (In GH CI). Is that expected?

@michalvavrik
Copy link
Member Author

There is a debug failure as well (In GH CI). Is that expected?

I can't tell anything from logs, I'll debug it now, but really, it can't have anything to do with changes this PR is making :-)

@michalvavrik
Copy link
Member Author

service name seems to be changed

We need to push upstream folks to document this. @michalvavrik , could you look into that?

I'm working on it, it's not trivial to determine what is it that changed, I already tried for 2 hours yesterday.

@michalvavrik
Copy link
Member Author

what has changed is that in 3.5.0.CR1 it for some reason matters what is set in amq.host property in app properties even though we override it with system property. I still can't connect it to any commit. will keep digging. anyway, this PR is right, service name was always as it sets

@michalvavrik
Copy link
Member Author

@fedinskiy quarkusio/quarkus#36549, can we merge this now?

@michalvavrik
Copy link
Member Author

I guess when the issue is fixed we can put it back, but this time with a comment. it took me while to realize that service name is incorrect, I wonder if that was intention.

@michalvavrik
Copy link
Member Author

I wonder if we should set OpenShift environment variables like amq-host instead of amq_host as defined in https://download.eclipse.org/microprofile/microprofile-config-2.0-M1/microprofile-config-spec.html#default_configsources.env.mapping

@fedinskiy
Copy link
Contributor

fedinskiy commented Oct 18, 2023

@michalvavrik we should, this is a problem with our property-to names pipelines, which was fixed here: https://github.com/quarkus-qe/quarkus-test-framework/pull/904/files

I presume, we need to process amqp properties the same way we process quarkus ones, so this bug should be fixed in OpenShiftClient, not in application properties

@michalvavrik
Copy link
Member Author

@michalvavrik we should no, this is a problem with our property-to names pipelines, which was fixed here: https://github.com/quarkus-qe/quarkus-test-framework/pull/904/files

I presume, we need to process amqp properties the same way we process quarkus ones, so this bug should be fixed in OpenShiftClient, not in application properties

This can only be bug if you ever intended to set incorrect name in app properties, in which case it should be noted in comment. It's just wasting of my time on this setting there something that is not supposed to work.

I'm going to fix it in FW then, ok.

@michalvavrik michalvavrik deleted the feature/fix-amq-and-bump-img branch October 18, 2023 11:30
@michalvavrik
Copy link
Member Author

This wasn't against you @fedinskiy , I just find it annoying to set there something that is not expected for usage, but has opposite intention without documentation.

@fedinskiy
Copy link
Contributor

fedinskiy commented Oct 18, 2023

@michalvavrik I just noticed, that I made a typo in my answer 😅

We can have anything we want in properties, but for environment variables, we should transfer it into proper format. Before 3.5, Quarkus was able to handle both formats, but this has changed since then

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