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

WFCORE-6808 Add capability-resolving variant of child ServiceTarget-based ServiceInstaller.Builder #5985

Merged
merged 1 commit into from May 2, 2024

Conversation

pferraro
Copy link
Contributor

@pferraro pferraro commented Apr 28, 2024

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 28, 2024
@wildfly-ci

This comment was marked as off-topic.

@pferraro pferraro changed the title WFCORE-6808 Add capability-resolving variant of child service target-based ServiceInstaller.Builder WFCORE-6808 Add capability-resolving variant of child ServiceTarget-based ServiceInstaller.Builder Apr 28, 2024
@yersan
Copy link
Collaborator

yersan commented Apr 29, 2024

@pferraro even for simple refactorings, I would say we should have them properly tracked in a valid Jira that, in a nutshell, explains the motivation/reason of the refactor. That would help to understand the change and track it down in the future.

What's your opinion about that?

@pferraro
Copy link
Contributor Author

pferraro commented Apr 29, 2024

WFCORE-6808 is the motivation for relocating this implementation. I'll update the commit message to make that more explicit.

@pferraro pferraro force-pushed the WFCORE-6808 branch 2 times, most recently from 8712bb1 to d29d722 Compare April 29, 2024 13:52
@yersan
Copy link
Collaborator

yersan commented Apr 29, 2024

@pferraro WFCORE-6808 or WFCORE-6806 (linked to this PR)? there are two commits, could you fix the message on both of them?

If this PR is for two Jiras I would expect:

  • two Jira links on the PR description
  • two Jira IDs in the PR title
  • each commit message with its Jira ID in the commit message.

@pferraro
Copy link
Contributor Author

pferraro commented Apr 29, 2024

There is only 1 jira - but I pasted the wrong link in the PR description. :/ Fixed.

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 13581 outcome was UNKNOWN using a merge of d29d722
Summary: Canceled (Error while applying patch; cannot find commit 848df8e in the https://github.com/wildfly/wildfly-core.git repository, possible reason: refs/pull/5985/merge branch was updated and the commit selected for the ... Build time: 00:00:27

@wildfly-ci
Copy link

Core -> Full Integration Build 13785 outcome was UNKNOWN using a merge of d29d722
Summary: Canceled (Error while applying patch; cannot find commit 848df8e in the https://github.com/wildfly/wildfly-core.git repository, possible reason: refs/pull/5985/merge branch was updated and the commit selected for the ... Build time: 00:00:17

@wildfly-ci
Copy link

Core -> Full Integration Build 13526 outcome was UNKNOWN using a merge of d29d722
Summary: Canceled (Error while applying patch; cannot find commit 848df8e in the https://github.com/wildfly/wildfly-core.git repository, possible reason: refs/pull/5985/merge branch was updated and the commit selected for the ... Build time: 00:00:20

@yersan
Copy link
Collaborator

yersan commented Apr 30, 2024

@pferraro Would you agree by squashing both commits or adding the WFCORE-6808 Jira ID to the first commit?

That would help to follow up on the changes in the future. Having a single commit without Jira ID makes finding the Jira that caused the change more difficult.

We recommend that each commit has the Jira ID on its message and I think it is a good practice even for simple refactorings.

@pferraro
Copy link
Contributor Author

Done.

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label May 2, 2024
@yersan yersan merged commit 5e147cf into wildfly:main May 2, 2024
12 checks passed
@yersan
Copy link
Collaborator

yersan commented May 2, 2024

Thanks @pferraro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
3 participants