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-19087]: External messaging resources can't be updated. #17694

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

ehsavoie
Copy link
Contributor

@ehsavoie ehsavoie commented Mar 6, 2024

  • misusage of context.isRuntimeOnlyRegistrationValid() which made all of the resources read-only

Jira: https://issues.redhat.com/browse/WFLY-19087

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.

@ehsavoie

This seems like it doesn't fully fix the misuse, more like it hides it?

All these definition classes end up with an field 'registerRuntimeOnly' that seems to not really mean 'registerRuntimeOnly'. I haven't looked at them all but it seems like it's "register as read only because ???".

I think the variable names should be corrected to reflect their actual use, javadoc added explaining the meaning of the params etc. Otherwise this is sure to get broken again when a maintainer thinks they understand but doesn't really and a code reviewer doesn't either.

Even now as a reviewer I'd need to investigate each of these classes to check that the changed to 'false' is right and I'd have to think too hard to do it.

@ehsavoie
Copy link
Contributor Author

ehsavoie commented Mar 8, 2024

@bstansberry I tried to make the intentions clearer by renaming parameters and adding some Javadoc.

 * misusage of context.isRuntimeOnlyRegistrationValid() which made all
   of the resources read-only.
 * updating the parameter names to make it clearer.

Jira: https://issues.redhat.com/browse/WFLY-19087

Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
@bstansberry bstansberry merged commit a829ae4 into wildfly:main Mar 15, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants