-
Notifications
You must be signed in to change notification settings - Fork 3k
Update to SmallRye Health 2.1.0 and MP Health 2.1 #4641
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of the PR and the commit message are off: they should be about Health, not Metrics.
Can you adjust? Thanks.
9b564d0
to
b250de1
Compare
Fixed @gsmet. Not quite sure how I managed that mixup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks! Let's wait for CI then.
@gsmet have you seen CI failures for documentation recently? That seems to be the reason for failure:
|
@FroMage could it be related to your PR? |
@kenfinnigan absolutely related. Where do you get those? We fixed this in the PR that introduced that bug, so the fix should be in. It depended on passing the right Maven CLI arguments. |
CI failed with that error Do I need to rebase? |
Well I don't think you can have that class without also having the fix. It involves passing the MAVEN_CMD_LINE_ARGS env var like @aloubyansky told me. CI passed for me with this. I don't get it. |
Yeah, see: Line 75 in b250de1
|
I can make a quick hack to catch the exception and bail out of generating that config file, but I'd like to know why this fails here. |
I'm very confused what's going on if I have the fix |
Do I have the right to push to your branch? |
Let's take this to zulip |
docs/pom.xml
Outdated
</arguments> | ||
<environmentVariables> | ||
<MAVEN_CMD_LINE_ARGS>${env.MAVEN_CMD_LINE_ARGS}</MAVEN_CMD_LINE_ARGS> | ||
<MAVEN_LOCAL_REPO>${env.MAVEN_LOCAL_REPO}</MAVEN_LOCAL_REPO> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try replacing it with <QUARKUS_LOCAL_REPO>${env.QUARKUS_LOCAL_REPO}</QUARKUS_LOCAL_REPO>
Nope, still an issue:
|
I'll try to debug it a bit later tonoght unless you want to disable the docs generation as @FroMage suggested. |
I've added a system.exit to end the main, so will see what happens |
MicroProfile TCK failing? That looks like something new. |
It's MP Config failing, which hasn't been updated at all. And it's failing because of:
Previously the build used to do a build with skip tests first. I wonder if that's changed |
@gsmet @FroMage @aloubyansky anyone has an idea why artifacts are suddenly not being found? MP Config TCK is failing but it's because it can't find the root tck directory artifact |
Oh, so it's not just our build artifacts then? |
I think it is just our build artifacts, as it's io.quarkus:quarkus-tck-parent:pom:999-SNAPSHOT But it's not part of docs build now. It's so weird |
Sorry, I meant that it's not related to my docs things now. Why haven't other PRs had this issue then? |
I have no idea, but it's so annoying! |
f095e98
to
72ec3ac
Compare
Nuts! @FroMage so my hack to work around docs caused another failure:
|
I am going to open a draft PR based on this one with the docs gen enabled just to debug the maven settings. I think if Stephane did it in a mojo it would work. It's painful to re-construct the maven settings in an external process. |
According to the logging that I added, the environment of the maven build process is inherited by the docs generating process. So specifying env vars in the config of the plugin is not necessary. I am still not sure what's missing though. |
Ok, I got it. The Maven resolver has enough info. The issue is that the docs are generated during |
Actually, sorry, that's not the actual reason. It's because our pipeline is configured to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I understand #4687 we can re-enable the doc generation in this PR, since it will work. It just has to run after we install the rest of the artifacts and not in the caching phase, which is the case now. |
Yes, this PR should only contain the Health upgrade in a single commit and things should work now. If it doesn't get cleaned up soon, I'll move it to the next release. |
164df80
to
9a4a85b
Compare
I've gone back to just the original commit and rebased on master |
Merged, thanks! |
No description provided.