Hacking on WildFly

Create a GitHub account if you don’t have one already Signing up for a new GitHub account

Create a Personal Access Token to work with on the command line. Account Security - Creating a personal access token

Create an account for Red Hat’s Jira [https://issues.redhat.com/] Choose "Sign up"

Fork wildfly repository into your account QuickStart - Fork a repo

Clone your newly forked copy onto your local workspace

Add a remote ref to upstream, for pulling future updates

As a precaution, disable merge commits to your main

The "formal" rules to format code are based on Eclipse. You can find them here WildFly Core IDE Eclipse Configuration

Same for cleanup and templates

There is a plugin for IntelliJ to use the Eclipse formatter: Eclipse Code Formatter

For component upgrades select this type for the Jira. The title must reflect the library and the new version. In the description please offer a links to the release notes, the git tag diff.

If you are aware that the component upgrade brings a fix for CVE, it’s good to note that in the JIRA title, as later that will make this information more visible in the WildFly release notes.

Create a simple topic branch to isolate that work

Rebase your branch against the latest main (applies your patches on top of main)

The -i triggers an interactive update which also allows you to combine commits, alter commit messages etc. It’s a good idea to make the commit log very nice for external consumption. Note that this alters history, which while great for making a clean patch, is unfriendly to anyone who has forked your branch. Therefore you want to make sure that you either work in a branch that you don’t share, or if you do share it, tell them you are about to revise the branch history (and thus, they will then need to rebase on top of your branch once you push it out).

If you need to first verify that your changes pass the checkstyle audit, do this first.

Then you can proceed with the build.

First try to run the tests as part of the build before sending a pull request.

Sometimes there are test failures that are not related to your code changes. Most times it’s your code change. Try to discuss this on Zulip.

You can get a full run using

This additional option will allow the build to continue even when there are test failures. Doing this, you can get a stock of all the test failures and figure out how many are related to your code changes.

The PR title should include a JIRA number directly from the project in question, whose corresponding JIRA issue will in turn have been linked to the pull request you are just now creating. The description should include a link to the JIRA. The description should also include a decent, human-readable summary of what is changing. Proper spelling and grammar is a plus!

Sample PR [WFLY-815] This is the sample title

Fixes: https://issues.redhat.com/browse/WFLY-815

It is highly annoying to reviewers when they find they’ve spent a great deal of time reviewing some code only to discover that it doesn’t even compile. In particular, it’s common for a patch to trip CheckStyle if it hadn’t been previously compile-tested at the least.

While it is tempting to rely on the automated CI/GitHub integration to do our build and test for us (and I’m guilty of having done this too), it generally just causes trouble, so please don’t do it!

This comes directly from [1], and I agree with it 100% (where the source document says "patch", think "commit"):

I also want to emphasize how important it is to separate functional and non-functional changes. The latter category includes reformatting (which generally should not be done without a strong justification).

We all know that development can sometimes be an iterative process, and we learn as we go. Nonetheless, we do not need or want a complete record of all the highs and lows in the history of every change (for example, an "add foobar" commit followed later by a "remove foobar" commit in the same PR) - particularly for large changes or in large projects (like WildFly proper). It is good practice for such change authors to go back and rearrange and/or restructure the commits of a pull request such that they incrementally introduce the change in a logical manner, as one single conceptual change per PR.

Note that this advice is not meant to discourage multiple commits in a single PR that are all steps on the way to an overall complex change. To the contrary, multiple well structured commits are sometimes critical to getting proper review of complex changes. For example a PR to refactor code away from an ill-fitting set of abstractions and to a new set can be difficult to review in a single commit. But doing so can be quite straightforward when broken up into, for example, four commits, one to make some small change to clean up something that would get in the way of the overall change, one introducing the new abstractions, one moving the implementation to the new abstractions, and one removing the old abstractions.

If a PR consists of dozens or hundreds of nontrivial commits, you will want to strongly consider dividing it up into multiple PRs, as PRs of this size simply cannot be effectively reviewed. They will either be merged without adequate review, or outright ignored or closed. Which one is worse, I leave to your imagination.

While in general it is my experience that WildFly contributors are good about this, I’m going to quote this passage from [1] regardless:

In addition, when something needs to be changed, the proper manner to do so is generally to modify the original commit, not to add more commits to the chain to fix issues as they’re reported. See Avoid massive and/or "stream of consciousness" branches.

It may come to pass that you have to iterate on your pull request many times before it is considered acceptable. Don’t be discouraged by this - instead, consider that to be a sign that the reviewers care highly about the quality of the code base. At the same time though, consider that it is frustrating for reviewers to have to say the same things over and over again, so please do take care to provide as high-quality submissions as possible, and see Pay attention and respond to review comments!

You don’t have to be an official reviewer in order to review a pull request. If you see a pull request dealing with an area you are familiar with, feel free to examine it and comment as needed. In addition, all pull requests need to be reviewed for basic (non-machine-verifiable) correctness, including noticing bad code, NPE risks, and anti-patterns as well as "boring stuff" like spelling and grammar and documentation.

If you review a PR and you feel you understand it in total and that it is correct, it is helpful to the WildFly mergers if you use the 'Approve' option discussed in the Github pull request review documentation. Don’t worry that your approval will trigger automatic merging; it won’t. It’s just easier for others to see that you regard the PR as correct if you use the Github workflow. (Please don’t, however, use the 'Approve' option if you are not expressing an approval of the PR overall; e.g. if you only looked at one part and made some comments that were addressed. Use comments for that kind of input.)

If you do review a pull request and make suggestions for changes, please do pay attention to the PR and try to acknowledge if your suggestions have been resolved. This is particularly important if it won’t be quickly obvious to others if your input was addressed.

When doing major and/or long-term refactors, while rare, it is possible that the above constraints become impractical, especially with regard to grouping changes. In this case, you can use a work branch on a (GitHub) fork of WildFly, applying the above rules in micro-scale to just that branch. In this case you could possibly ask a reviewer to also review some or all of the pull requests to that branch. Merge commits would then be used to periodically synchronize with upstream.

In this way, when the long-term branch is ready to "come home" to the main branch, the reviewers may have a good idea that the (potentially quite numerous) changes in the work branch have been reviewed already.

[1]: https://www.kernel.org/doc/Documentation/process/submitting-patches.rst