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 build-time config detection for relocated properties and for config properties from extension deployment modules #1154

Merged

Conversation

michalvavrik
Copy link
Contributor

@michalvavrik michalvavrik commented Jun 3, 2024

Summary

closes: #1149

the script is based on https://github.com/quarkusio/quarkus/blob/main/docs/src/main/java/io/quarkus/docs/generation/AllConfigGenerator.java

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
Contributor Author

run tests

@michalvavrik michalvavrik force-pushed the feature/fix-build-time-config-detection branch from e990536 to bc49272 Compare June 3, 2024 21:54
@michalvavrik
Copy link
Contributor Author

run tests

@michalvavrik
Copy link
Contributor Author

JVM build - Latest Version also fails over it in a daily build.

@rsvoboda
Copy link
Member

rsvoboda commented Jun 4, 2024

How often do we need to have the list updated? Is there any noticeable milestone?

Maybe adding a note into https://github.com/quarkus-qe/quarkus-test-framework/blob/main/RELEASE.md#preconditions for new major/minor releases?

CC @mjurc

@michalvavrik
Copy link
Contributor Author

michalvavrik commented Jun 4, 2024

How often do we need to have the list updated? Is there any noticeable milestone?

strange how it is most of the build-time and build-time-fixed properties are placed in classes in the runtime modules (for build-time it's waste...). and there, it is still automated. then it is not going to fail as long as other bulid-time property or custom class/dependency is detected.

so I'd say:

  • you only need to update it if you need to add a property there because your newly added test is failing
  • in theory sometimes build-time properties can be moved to runtime, so it would be useful to update it from time to time so that we don't force custom build; this scenario is very unlikely and I wouldn't spend time on it unless I had it

Maybe adding a note into https://github.com/quarkus-qe/quarkus-test-framework/blob/main/RELEASE.md#preconditions for new major/minor releases?

Honestly, we had build-time list for years and we only updated it when we had failing tests. I suppose I could write a workflow that updates it periodically but we don't have this kind of time.

I think we should update it when there is a failure and then when we have a time.

JVM build failure goes down to the quarkusio/quarkus#36826

@rsvoboda
Copy link
Member

rsvoboda commented Jun 4, 2024

Cool, seems it's fine to have this as adhoc action.

strange how it is most of the build-time and build-time-fixed properties are placed in classes in the runtime modules (for build-time it's waste...)

Worth GH issue?

@michalvavrik
Copy link
Contributor Author

strange how it is most of the build-time and build-time-fixed properties are placed in classes in the runtime modules (for build-time it's waste...)

Worth GH issue?

I run quick check and only found 6 examples of this. However additionally, build-time-runtime-fixed is placed in the runtime so it is auto-detected.

as for opening issue - sadly config classes are treated as part of the API now, which is one of the reasons why there is so long process of migrating config classes to config mapping interfaces (in addition to reflection registration effectiveness), so:

  • they won't be ale to move it in case other extension is using it (either quarkiverse or custom)
  • they can say it when config phase changes they don't have to change class package (move it)

@michalvavrik michalvavrik requested a review from mjurc June 4, 2024 08:31
@galderz
Copy link

galderz commented Jun 4, 2024

JVM build failure goes down to the quarkusio/quarkus#36826

What failure? The CI failure?

@michalvavrik
Copy link
Contributor Author

JVM build failure goes down to the quarkusio/quarkus#36826

What failure? The CI failure?

We have one build failure in the JVM build - Latest versions of this PR CI with message Could not find goal 'native-image-agent' in plugin io.quarkus.platform:quarkus-maven-plugin:3.11.0 among available goals , my colleague @jedla97 described it in here quarkusio/quarkus#40951.

@galderz
Copy link

galderz commented Jun 4, 2024

JVM build failure goes down to the quarkusio/quarkus#36826

What failure? The CI failure?

We have one build failure in the JVM build - Latest versions of this PR CI with message Could not find goal 'native-image-agent' in plugin io.quarkus.platform:quarkus-maven-plugin:3.11.0 among available goals , my colleague @jedla97 described it in here quarkusio/quarkus#40951.

Not a bug

@michalvavrik
Copy link
Contributor Author

JVM build failure goes down to the quarkusio/quarkus#36826

What failure? The CI failure?

We have one build failure in the JVM build - Latest versions of this PR CI with message Could not find goal 'native-image-agent' in plugin io.quarkus.platform:quarkus-maven-plugin:3.11.0 among available goals , my colleague @jedla97 described it in here quarkusio/quarkus#40951.

Not a bug

got it, thanks for your time

Copy link
Member

@mjurc mjurc left a comment

Choose a reason for hiding this comment

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

LGTM - just verifying - the CI failure is caused by wrong Quarkus platform bom version in action?

@michalvavrik
Copy link
Contributor Author

LGTM - just verifying - the CI failure is caused by wrong Quarkus platform bom version in action?

we need the plugin to use correct version, Jakub tries to address it here #1157 (I'll review soon); I wouldn't be worried about this failing as this PR is not specific to Quarkus version and released version was green.

@mjurc mjurc merged commit 0110419 into quarkus-qe:main Jun 4, 2024
9 of 10 checks passed
@michalvavrik michalvavrik deleted the feature/fix-build-time-config-detection branch June 4, 2024 20:08
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.

Build time properties incorectly handles .withProperty
4 participants