Skip to content

Github Guidelines

Jump to bottom Edit New page
Greg Schueler edited this page Jul 18, 2019 · 5 revisions

Github Development Guidelines

A few guidelines for best practice use of Github for Rundeck development. These aren't strict rules, sometimes they can be bent or ignored, but attempt to follow as close as possible to these suggestions.

Please take the time to read all the guidelines, they make group development of the project easier.

Git

Familiarize yourself on how branches, remotes, merging and rebasing work. See: Pro Git Book.

Commit Messages

  • Each commit message should be a comment about the reason for the changes.

Each commit message will eventually show up to some future developer as attached to a line in a file. They will see a line and ask "why is this like this?", and using git annotate/blame or their IDE, they will look at who wrote the line (you) and why it was written (your commit message).

Hopefully your commit message answers their question. Often that developer is also you. So do yourself a favor and write a good commit message.

  • The commit message should describe why not what.

We all know what was changed; we can see the change in git. What we don't know by looking at the change itself was the reason why you made the change.

Examples of bad commit messages:

  • set x to 12 (don't describe the change, tell why you changed it)
  • checkpoint

(Some more guidelines here.)

Commit Hygiene

What is "commit hygiene"? It is being clean in your commits. It is a corrollary of having useful commit messages.

What if there are a lot of changes, and I can't summarize all these changes in a simple commit message?

That means you are committing too many changes at once, and need to clean it up:

  • Separate unrelated changes into discrete commits that contain related changes.

This may be tricky to do from the commandline. Use a tool like Gitup or your IDE, which will make it easier to "stage" smaller sets of related changes for each commit. Gitup makes it really easy to do this line by line even.

What counts as "unrelated"? Isn't "Updating the Widget behavior" a single related change?

Again, think about commits as comments on each changed line in a the changed files. All the changes might be part of a single feature, but usually the individual lines and files that are changed are not all part of a single "related" commit. You might include a single file in one commit, a few lines from different files in another, and so on.

Another helpful way to look at it is: if someone else checks out this exact commit, would the code build? That is, have your commits been cohesive enough that each change is an incremental, working change to the codebase? If you answer no, you might make sure you are separating related changes into the same commit.

(Re)Formatting code and non-functional changes

This is related to Commit Hygiene:

  1. Avoid reformatting code when performing other changes.
  2. If you need to reformat something, do it in a discrete commit.
  3. To avoid having your code reformatted, please ensure that you are utilizing both .editorconfig and .eslintrc.js files located in grails-spa directory. See the section titled "JavaScript Coding Standard" below.

For example, if the source is using tabs and you want to convert it to spaces, do that in a separate commit before or after other impactful code changes; the reformatting is unrelated to the other changes.

What if I don't?

Reviewers of your PR, and future readers of the commit history, will have a hard time discerning what was changed. If you must reformat, do it in a discrete commit, so that reviewers can look at the pertinent commit changes separately from the format changes.

Sample commit message: cleanup: reformat via IDE

JavaScript Coding Standard - Code Formatting for Vue and JS files in rundeckapp/grails-spa director

To ensure that the code is formatted per spec, we employ both an .editorconfig and eslintrc.js file. The contents of these files may change from time to time, but their use when adding/editing Vue or JS files that reside in the grails-spa directory is practically a requirement. Ensure that you are loading the .editorconfig file in your IDE as well as running npm run start while making changes to files in the grails-spa directory. The ensures that your code (changes) are being properly limited and that the formatting is aligned with the StandardJS ruleset.

For more information about the StandardJS ruleset, please visit the StandardJS github page.

Milestones

We create a Github Milestone for each Release. We use the milestones as a way to generate release notes once we perform a Release, so it's important that all Issues fixed in the release, and all Pull Requests that are part of the release be assigned to the correct Milestone.

Github Issues

General guidelines:

DO

  • Assign a label of bug or enhancement if appropriate
  • For bug reports: include Version information, info on how to reproduce it, etc (see github issue template)
  • If you are writing/editing Vue or JS files that reside in the rundeckapp/grails-spa directory, make sure to load the .editorconfig and eslintrc.js.

Pull Requests

DO

  • Use a descriptive title for the PR. "Fixes the kill button not working #123". (Not "Fixes #123")

    What if I don't?

    Nobody knows what it does without clicking it

  • For bug fix/enhancements, reference the fixed issue number in the title.

    What if I don't?

    Hard to tell which issue it is addressing without clicking it, or if the issue is not referenced in the commit/description

  • Assign a reviewer to the PR, and to the pulse in Monday

    What if I don't?

    Without assigning it, it is less likely to get reviewed soon. Even if you assign the "wrong" person, they can reassign it to someone appropriate, so better to assign it than not.

  • Write documentation for new features/configurations. (See [Documentation] section)

    What if I don't?

    Without documentation, a new feature may not be visible to users.

Example:

Pull Request
Title: Fixes the kill button not working on the execution page #123
Body: 
       Fixes #123
       
       typo in javascript
       
Milestone: 3.1.0

Commit Message: Fix #123 kill button not working

Bug Fixes

DO

  • Use the phrase "Fixes #123" either in the commit message or the PR description. You can also use other keywords. This will mean the referenced issue is automatically closed when the PR is merged.

    What if I don't?

    The issue remains open even after it is fixed and the PR was merged.

Merging

DON'T:

  • Merge a PR if any Checks fail.

    What if I do?

    You have introduced a change to master that may break someone else's PR or local build

  • Merge a PR without assigning it to a Milestone (See above).

  • Merge a PR without it having been Approved by a reviewer.

Documentation

DO

  • Create a PR against the primary branch in the https://github.com/rundeck/docs repo to add documentation about the new behavior or/configurations, especially if your change is an enhancement that is not end-user visible in the GUI. See the Docs repo for more information.

Backporting

Sometimes we need to backport changes to one or more previous maintenance releases.

Procedure:

  1. Tag the original fix PR as "needs-backport"
  2. File a new Issue, titled such as "[2.0.x] Backport: (original title)" including the original issue title and the target maintenance branch
  3. Create the backport:
    1. checkout the maint-V.0.x branch for the target minor version
    2. cherry-pick the commits from the original fix
    3. create a new PR, target the maint-V.0.x branch with your fixes
  4. Add the PR to the appropriate Milestone
Add a custom footer
Add a custom sidebar
Clone this wiki locally