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.

Sign up for GitHub

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

Jump to bottom

WFCORE-6321 Expose ServerEnvironment via a capability #5480

Merged
merged 1 commit into from
Aug 4, 2023
Merged

WFCORE-6321 Expose ServerEnvironment via a capability #5480

merged 1 commit into from
Aug 4, 2023

Conversation

parsharma
Copy link
Contributor

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 28, 2023
@darranl
Copy link
Contributor

darranl commented May 31, 2023

/retest

@wildfly-ci

This comment was marked as outdated.

Sign in to view
yersan
yersan requested changes Jun 28, 2023
View reviewed changes
server/src/main/java/org/jboss/as/server/ServerEnvironmentService.java Outdated Show resolved Hide resolved
server/src/main/java/org/jboss/as/server/ServerEnvironmentService.java Outdated
@@ -69,6 +72,9 @@ public static void addService(ServerEnvironment serverEnvironment, ServiceTarget
.install();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the service name is deprecated, you should replace their usages with the capability: when installing the ServerEnvironmentService, we have to use the capability service name. You should also review any SERVICE_NAME usages. When deprecating something, we should add always a comment about what's the alternative.

If the capability name differs from the service name, we should add an alias when installing the service using the capability.

server/src/main/java/org/jboss/as/server/ServerEnvironmentService.java Outdated Show resolved Hide resolved
@yersan
Copy link
Collaborator

yersan commented Jun 28, 2023

@parsharma we need the Jira Issue on the commit message and PR title.

@parsharma
Copy link
Contributor Author

@yersan sorry, I was busy on 7.4.12 GA. I will update the PR soon.

@wildfly-ci
Copy link

Core -> Full Integration Build 12571 outcome was FAILURE using a merge of 3be5b14
Summary: Exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:04:46

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 12653 outcome was FAILURE using a merge of 3be5b14
Summary: Exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:05:08

@wildfly-ci
Copy link

Core -> Full Integration Build 12772 outcome was FAILURE using a merge of 3be5b14
Summary: Tests passed: 1439, ignored: 18; exit code 1 (Step: Build & test full (Maven)) (new) Build time: 00:09:40

@yersan
Copy link
Collaborator

yersan commented Jul 13, 2023 •
edited
Loading

@parsharma errors are legit.

You still need to address all the pending points commented on above: add a Jira number to the PR title, use an alias for the service, add a comment to the @deprecate annotation, review any usage of the current service server/src/main/java/org/jboss/as/server/ServerEnvironmentService.java#SERVICE_NAME and replace it with the capability, ensure the capability is correctly registered. In addition, when done, please squash the commits in a single one.

If you are pushing changes but you still think there is pending work to do, please, move the PR to a draft so we can ignore its review until is done from your side, thanks!

@parsharma parsharma changed the title Expose ServerEnvironment via a capability WFCORE-6321 Expose ServerEnvironment via a capability Jul 13, 2023
@parsharma
Copy link
Contributor Author

@yersan I will change it to draft because I am still working on it. Thanks for the review comments. I will update the code soon.

@parsharma parsharma marked this pull request as draft July 13, 2023 08:58
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@parsharma parsharma force-pushed the WFCORE-6321 branch 2 times, most recently from a9d0705 to 8f3a253 Compare July 18, 2023 13:13
@parsharma parsharma marked this pull request as ready for review July 18, 2023 13:14
yersan
yersan requested changes Jul 20, 2023
View reviewed changes
Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Minor detail about the import, but looks good to me

server/src/main/java/org/jboss/as/server/ServerEnvironmentService.java Outdated Show resolved Hide resolved
pferraro
pferraro approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@pferraro pferraro left a comment

Choose a reason for hiding this comment

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

All looks good now. Thank you for persevering @parsharma !

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Aug 4, 2023
@yersan yersan merged commit 48584a4 into wildfly:main Aug 4, 2023
1 check passed
@yersan
Copy link
Collaborator

yersan commented Aug 4, 2023

Thanks, @parsharma and @pferraro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yersan yersan yersan approved these changes

@gaol gaol gaol left review comments

@pferraro pferraro pferraro approved these changes

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@parsharma @darranl @wildfly-ci @yersan @pferraro @gaol