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

[#3547] Migrate to Quarkus 3.2.6.Final #3556

Merged
merged 1 commit into from
Oct 9, 2023
Merged

[#3547] Migrate to Quarkus 3.2.6.Final #3556

merged 1 commit into from
Oct 9, 2023

Conversation

harism
Copy link
Contributor

@harism harism commented Oct 1, 2023

This PR is still slightly in progress.

  1. Increased Quarkus version to 3.2.6.Final
  2. Fixed all errors so that following normal image build and push succeeds:
mvn clean install -Pbuild-docker-image,metrics-prometheus,docker-push-image -Ddocker.*.details for harrismatt repository
  1. Ran integration tests successfully with previous step images:
mvn verify -Prun-tests -Dhono.deviceregistry.type=file -Ddocker.image.org-name=harrismatt

Native image build is totally untested yet but there is so many changes already I think it's better make a PR now.

Please keep me posted on any improvements there might be and questions too.

@harism
Copy link
Contributor Author

harism commented Oct 3, 2023

Is it possible to run Hono native image integration tests from this PR code? It would ease my work with native image support a lot if I would get results from automated build here.

@sophokles73
Copy link
Contributor

Wow, thanks for working on this 👍

Most of the changes seem to be related to simply replacing the package names (which is good). Anyway, since there are quite some files affected, I will need some time to review ...

@sophokles73
Copy link
Contributor

Is it possible to run Hono native image integration tests from this PR code?

Well, you can always run them on your workspace, right?

@harism
Copy link
Contributor Author

harism commented Oct 4, 2023

Is it possible to run Hono native image integration tests from this PR code?

Well, you can always run them on your workspace, right?

Well I am having issues with native tasks on both my Windows and macOS dev environments actually;

  • On Windows, the native image build produces images where the actual built runner executable is not marked with +x on image filesystem. I would need to fix those manually always chmod a+x for the runners to run integration tests with these images.
  • On macOS the whole docker-in-docker native build step fails because it cannot find docker command.

Anyway, I can still compile the native images on Windows and see that succeeds at least and what warnings etc there might be.

@sophokles73
Copy link
Contributor

Ok, so you are not able to run the integration tests locally on your dev machine(s) and that is why you would like the https://github.com/eclipse-hono/hono/actions/workflows/native-images-tests.yml workflow to be run on your pull request, did I get that right?

@harism
Copy link
Contributor Author

harism commented Oct 5, 2023

Ok, so you are not able to run the integration tests locally on your dev machine(s) and that is why you would like the https://github.com/eclipse-hono/hono/actions/workflows/native-images-tests.yml workflow to be run on your pull request, did I get that right?

In a sense yes, but I am not familiar with these Github Actions at all, and I am rather asking is it possible to run this PR changes on that integration test pipeline you linked at all. And if it is it would be enough to do it only once there is changes in this PR, on a request basis for example.

@sophokles73
Copy link
Contributor

Ok, got it. Let me see what I can do ...

@harism
Copy link
Contributor Author

harism commented Oct 5, 2023

Ok, got it. Let me see what I can do ...

I pushed the changes which works on my dev environment to build the native Hono images and there is some native image compiler check warnings only if I made the commit ok. I have not had time to run native image integration tests on these changes yet though.

@sophokles73
Copy link
Contributor

@harism sorry for messing up your branch. I was trying to fix a conflict with the master branch using GitHub's web based tooling. However, that resulted in a merge commit which I'd rather not have.

Can you please remove my commit from your branch and rebase on current master's HEAD revision?

@harism
Copy link
Contributor Author

harism commented Oct 6, 2023

@sophokles73 no worries, I did those steps you asked to.

@sophokles73
Copy link
Contributor

I am afraid your latest commit didn't do anything at all.
I was hoping you could reset your branch to your first commit from yesterday and then rebase it.

@harism
Copy link
Contributor Author

harism commented Oct 6, 2023

@sophokles73 you're correct, I didn't do the rebase on master, hope it's better now.

bom/pom.xml Outdated Show resolved Hide resolved
bom/pom.xml Show resolved Hide resolved
bom/pom.xml Show resolved Hide resolved
bom/pom.xml Outdated Show resolved Hide resolved
bom/pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
services/parent/pom.xml Outdated Show resolved Hide resolved
services/parent/pom.xml Outdated Show resolved Hide resolved
clients/pubsub-common/pom.xml Outdated Show resolved Hide resolved
@harism
Copy link
Contributor Author

harism commented Oct 6, 2023

@sophokles73 thank you for the thorough review effort, I think I got all your comments fixed. Truthfully, those comments you needed to mention multiple times why not use Quarkus default version, it just did not occur to me to remove whole depency and its version setting in bom/pom.xml until you told me to.

@sophokles73
Copy link
Contributor

@harism can you please make sure to use an email address in your commit that matches your Eclipse account, otherwise the ECA check will fail. Please refer to https://www.eclipse.org/projects/handbook/#resources-commit for details ...

@sophokles73 sophokles73 added this to the 2.5.0 milestone Oct 7, 2023
@sophokles73 sophokles73 linked an issue Oct 7, 2023 that may be closed by this pull request
@sophokles73 sophokles73 removed a link to an issue Oct 7, 2023
@sophokles73
Copy link
Contributor

@harism this looks pretty good 👍

Would you mind adding yourself and/or the organization you are affiliated with to the list of copyright holders in legal/src/main/resources/legal/NOTICE.md?

@calohmn IMHO we should take a closer look at the changes to the SamplerProducer and TracerProducer after merging this PR. WDYT?

@harism
Copy link
Contributor Author

harism commented Oct 7, 2023

@sophokles73 I will add copyright holder as part of [#3557] PR since it was first in queue to submit this change.

@calohmn
Copy link
Contributor

calohmn commented Oct 7, 2023

IMHO we should take a closer look at the changes to the SamplerProducer and TracerProducer after merging this PR.

@sophokles73 I'm currently having a look. There are some changes needed - they could be integrated in this PR still, I think.

@calohmn
Copy link
Contributor

calohmn commented Oct 7, 2023

A note on the property names in the SamplerProducer class:

    static final String PROPERTY_OTEL_SERVICE_NAME = "otel.service.name";
    static final String PROPERTY_OTEL_TRACES_SAMPLER = "otel.traces.sampler";
    static final String PROPERTY_OTEL_TRACES_SAMPLER_ARG = "otel.traces.sampler.arg";

My intuition was to use the corresponding quarkus.otel.* properties now instead.
But there is an issue with the quarkus.otel.traces.sampler.arg property, leading to an error when running the integration tests with tracing enabled (using the jaeger_remote sampler) - see quarkusio/quarkus#36335.
Therefore I think we should stick with the otel.* properties. That also means there are no configuration changes needed in this regard when updating to Hono 2.5.

@calohmn
Copy link
Contributor

calohmn commented Oct 9, 2023

Looking good from my point of view 👍

Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks a lot for your time and effort @harism 👍
We really appreciate it!

@sophokles73 sophokles73 merged commit a30228a into eclipse-hono:master Oct 9, 2023
7 checks passed
@harism harism deleted the migrate_to_quarkus_3.2.6 branch December 18, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants