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

WFLY-17047 Weld Probe removal #16114

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Sep 27, 2022

JIRA - https://issues.redhat.com/browse/WFLY-17047

This PR does not contain Weld version update yet, just changes related to Probe removal
EDIT: Weld 5.1.0 is now syncing with Central and this PR will contains the updated version as well.

@manovotn manovotn changed the title Remove probe WFLY-17043 Weld Probe removal Sep 27, 2022
@manovotn manovotn changed the title WFLY-17043 Weld Probe removal WFLY-17047 Weld Probe removal Sep 27, 2022
@wildfly-ci
Copy link

Hello, manovotn. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@manovotn
Copy link
Contributor Author

Since there was no other than Alpha release of WFLY, I have decided it would be cleaner to change weld subsystem 5.0 to not accept devel mode property at all.
However, this draft is currently failing in WeldSubsystemTestCase because older subsystem versions can have this property.
Any hints on what's the proper way to handle this @bstansberry? I tried looking into WeldTransformers, but that seems to be for the opposite case (where you have newer config and need to adapt it to older) - but I am probably just missing something, right?

@github-actions
Copy link

Dependency Tree Analyzer Output:

Removed Dependencies:

  • org.jboss.weld.probe:weld-probe-core:jar:5.0.1.Final:compile

CC @wildfly/prod

@github-actions github-actions bot added the deps-changed Dependencies have been checked, and there are changes highlighted in a comment label Sep 27, 2022
@darranl
Copy link
Contributor

darranl commented Sep 27, 2022

I have had to do what Brian is suggesting here where I have been disabling the security-realm attribute across subsystems e.g.

#16107

In my case I also report an error at runtime if set but that is probably a step too far for this attribute.

@manovotn
Copy link
Contributor Author

What about the test failures I mentioned above? I am not quite grasping how are tests such as this supposed to behave when an attribute is being removed from newer version.

@manovotn
Copy link
Contributor Author

I'll get back to this later today.
Looking at @darranl PR, I think I understand what you meant. Latest version of schema still needs to support dev mode attribute, but it will be a no-op.

@manovotn
Copy link
Contributor Author

The PR is now changed to remove Probe as a dependency but it retains Weld devel mode configuration option as it was, only deprecates it.
Is this what you meant @bstansberry @darranl ?

@manovotn manovotn marked this pull request as ready for review September 29, 2022 12:58
@manovotn
Copy link
Contributor Author

manovotn commented Sep 29, 2022

Hello, manovotn. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

BTW I don't think I can trigger this either :)

@bstansberry
Copy link
Contributor

/ok-to-test

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

This LGTM @manovotn

As @darranl said it's possible to throw an OperationFailedException if this is set to true, e.g. at https://github.com/wildfly/wildfly/blob/main/weld/subsystem/src/main/java/org/jboss/as/weld/WeldSubsystemAdd.java#L88, but I think that's overkill. (That code only executes in a server that's running in normal mode.) Doing that would interfere with people using a particular config in different dev environments, without adding a ton of value vs the WARN that gets logged.

@bstansberry bstansberry added the ready-for-merge Only for use by those with merge permissions! label Oct 4, 2022
@bstansberry bstansberry merged commit adbb18c into wildfly:main Oct 4, 2022
@bstansberry
Copy link
Contributor

Thanks @manovotn!

@manovotn manovotn deleted the removeProbe branch October 4, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-changed Dependencies have been checked, and there are changes highlighted in a comment ready-for-merge Only for use by those with merge permissions!
Projects
None yet
4 participants