handbook.md

layout	title
contribute	Handbook

How we make things

Branching and SCM

We use git for source control. The core projects:

• Foreman
• Smart Proxy
• Foreman installer
• Foreman SELinux)

follow this model for branching:

• develop
  • New pull requests should be made against this branch.
  • All pull requests are merged here.
  • Contains nightlies code. It's relatively stable, but we make no promises about it.
  • test_develop in Jenkins ensures our tests are passing at all times for various configurations.
• release-stable (e.g. 1.10-stable) -
  • Individual developers do not need to maintain this, it's part of our release process
  • Code is cherry-picked using git cherry-pick -x into this branch.
  • It contains the latest stable code for a release cycle, 1.8-stable contains 1.8.1, 1.8.2, 1.8.3...
  • Typically, we will announce the start of a new branch.

And this is how these projects tag each stable release point:

• release (e.g. 1.10)
  • Code is cherry-picked using git cherry-pick -x into this tag.
  • It contains the latest stable code for a specific release, for instance, 1.10.0.

We do not force push to any of these branches. If a commit must be reverted, use git revert, and push the reverted commit.

Core projects have a linear history (like this). A way to achieve this is to never create merge commits, by always rebasing your changes with develop git pull --rebase upstream develop. This allows us to easily revert our code to any point in time, which is helpful to detect in what commit we introduced an error using git bisect.

We encourage developers to adopt this strategy for managing their repositories because git linear history makes it very easy to rollback or reverse a certain commit without getting conflicts. It also makes it easier to cherry pick commits for a stable release.

Changelog

Changelogs are generated by scripts, see an example here

Coding standards

The purpose of the Foreman coding standards is to create a baseline for collaboration and review within various aspects of the Foreman project and community, from core code to plugins.

Coding standards help avoid common coding errors, improve the readability of code, and simplify modification. They ensure that files within the project appear as if they were created by a single person.

Pull requests

All pull requests need to have an associated issue in the Foreman issue tracker.

Foreman's PR processor will parse all pull requests, assign labels, and run tests for all major projects. Pull requests are always rebased on top of the develop branch so that the git log stays linear.

Commit messages

Provide a brief description of the change in the first line (50 chars or less), including a issue number.

The title must start with Fixes #xxxx. If your fix extends a previous fix that has not been shipped in a release, use Refs #xxxx. Our PR processor will understand these formats and will auto associate the pull request with the issue in the tracker.

You can use the following model:

Fixes #9999 - launch mars probe in V2 API

Some collaboration between teams will be necessary to accomplish this
task. Other features, #2932 and #2933 will be closed after this. V1
changes to the API were not necessary.

Insert a single blank line after the first line.

Optionally, include more detailed explanatory text if necessary and wrap it to about 72 characters or so. On some editors, git commit automatically wraps the text to these limits as you type in.

Ensure the description is of the change and not the bug title, e.g. "X now accepts Y when doing Z" rather than "Z throws error".

####Notes

• Some background to justify this commit message style can be found here.
• By adding 'Refs #' PR processor will auto add the commit to an existing issue. Usually an already closed issue, or just to add some code to a existing issue with another PR open.
• Only use "refs" when when adding to a commit that has not already shipped. If it has shipped in a release already, please file a new issue and use "fixes".

Ruby

We follow the Ruby Style Guide and the Rails Style Guide. We use Rubocop (in Jenkins) to enforce most of these rules. New projects such as plugins should enforce all Rubocop rules and disabling them should be done under a very specific circumstances.

####Do

• Use blank? over empty? for strings.
• Keep in mind models need to be filtered through the scope authorized.
• If you feel like you are nearly copy pasting code, please refactor.
• Raise exceptions of the type Foreman::Exception
• Ensure your migrations can be reversed properly.
• Use Rails 3+ validators syntax, e.g: validates :name, :uniqueness => true, :presence => true
• Extract common validators to a validator class reused by different models.
• View templates must have an .html.erb extension.
• View templates use %> instead of -%>
• Make sure Mixins use ActiveSupport::Concern fully, with no class_eval, InstanceMethods etc.
• New fields in your model must be documented in the latest version of the API if it exists.
• Ensure Apipie documentation is correct, required fields, names of parameters and HTTP methods.
• Add appropriate permissions for non-admin users and test your routes with them.
• Set :mark_translated: true in config/settings.yaml to spot missing string i18n extractions.
• Concerns and new classes are added to app/, not lib/.
• Favor .present? over .nil?. Only use the latter if you are truly checking for nil, which is uncommon.
• If adding new model attributes, use scoped search definitions where appropriate.
• Virtual fields must have the option :only_explicit when added to scoped search.
• Only catch exceptions that are expected. Wrap them in Foreman::WrappedException
• When you need to catch a Foreman::Exception make sure the ERF code is displayed to the user. Send a pull request to theforeman.org with possible fixes for the ERF code.
• Include unit tests for public methods in models, helpers or concerns, tested in isolation.
• Include functional tests for public methods in controllers
• All new APIs, or additional model attributes have apipie documentation and are in API views
• API functional tests have a valid test first, followed by invalid tests
• Pass block to logger.debug to lazily evaluate expensive debug statements
• No new "stylesheet" tags are added to views, they're already in app.css
• Deprecations use Foreman::Deprecation with a deadline of latest stable + three
• Use :success, :not_authorized, etc..., instead of actual HTTP status codes.

####Don't

• Use .to_sym, .send, eval or other reflection on untrusted inputs.
• Catch unexpected exceptions, or Foreman::Exception.
• Use single character variable names or abbreviations. The keyboard erosion you might save by doing that is not worth it.
• Squash exceptions

Tests

We use MiniTest::Unit syntax.

• Use test 'description' do instead of def test_description
• Use custom assertions for variable status checks, e.g: assert_empty variable instead of assert variable.empty?. These assertions produce errors that are easier to read.
• Use :success, :not_authorized, etc..., instead of actual HTTP status codes.
• Favor FactoryGirl over fixtures. Only use fixtures when the object is created very often throughout the test suite.
• When using FactoryGirl, do not use create unless necessary, and use build instead. The former will save the object to the database (slow), the latter keeps the object in memory (faster and garbage collectable)
• If your tests are repeating code except for a couple of changes, wrap it in a context block, or write a helper that takes in the changes through arguments and generates the tests.
• Use stubs and mocks extensively to test external calls, slow calls not related with the actual test, and also to avoid testing framework features. They make our test base much faster.
• Use setup_user if you need to create an user with special permissions and roles instead of doing it yourself in the test.
• For functional tests, if several controllers share the same behavior, extract it to a shared functional concern

Strings and translations

Be mindful of our Translating guide. A quick overview:

Ruby

• To translate a string use _('My string')
• To translate string with a parameter use _("String with param: %s") % param
• To translate string with more than one parameters do not use _("Params: %s and %s") % [param1, param2] which is translator-unfriendly
• Therefore for more than one parameters use _("Params: %{a} and %{b}") % {:a => foo, :b => bar}
• Note that parameters are not translated. Variables should not contain text that needs translation.
• To mark something for translation (but not translate) use N_("String")
• Ensure you later translate the text before it's displayed with _(variable)
• To use plural form use n_("One", "Two", number) - note this function always accepts three parameters as the base language is usually English but translators are able to define as many plural forms as they need.
• Plural forms are usually used with one parameter, do not forget to add trailing parameter to it: n_("%s minute", "%s minutes", @param) % @param
• When using strings with ERB (e.g. for deface gem), escape properly: _('Test <%%= %{var1} %%>') % { :var1 => "xxx" }
• Note that gettext does not extract from interpolation - this will not work: "blah #{_('key')} blah"

JavaScript

• Both __ and n__ functions are available and work in much the same way as in Ruby, with the following exceptions..
• Interpolation of values uses Jed.sprintf on the translated string, so for a single parameter: Jed.sprintf(__("Foo: %s"), "bar")
• Multiple parameters must be named: Jed.sprintf(__("Example: %(foo) %(bar)"), {foo: "a", bar: "b"})

Code reviews

We review pull requests on GitHub. Most projects have one or two maintainers that review pull requests. Everyone is invited to review code.

Contributors

Write a good description, a good title, and explain why the change is necessary. A line or two explaining the problem, the solution, and a way to reproduce the issue (extra points if it's a script) help enormously.

Assume reviewers have no idea or background about your patch. Even if the usual reviewers know you personally and you know they know why your change is necessary, maybe not all reviewers are aware of it. Furthermore, after some time, they might not remember well, and new reviewers will not be able to review your code without background and a good explanation.

Generally, if you want to submit significant changes to the code, discuss it first on #theforeman-dev (Freenode) or Foreman-dev (mailing-list). If you know who are the usual maintainers for the code you want to change, try to ask them to validate your assumptions, your design, and if they can, ask them to review your code. This will save you a lot of going back and forth with reviewers that do not understand the reasoning behind your pull request.

Submit changes incrementally. If you are submitting a pull request that will break compatibility with older APIs, spend the effort to make it compatible first. If you think your feature is not ready for prime time yet, it's OK to submit small changes and hide the feature using feature flags.

Feel free to bring the attention of reviewers by calling them using '@' on GitHub.

Remember the old saying, you are not your code. Reviewers will be consistent with style and good practice, and it's important to know you're not being criticised personally when that happens.

Reviewers

How to become a reviewer

Simply start reviewing patches - as detailed above, all patches are available publicly as GitHub Pull Requests. Use the guidelines below to assist you in your review, and don't be afraid to question. We all started out not knowing the code, and reviews are a good way to learn why things are done a certain way.

Style

Be open to change, but respect the borders between core and plugins. Refer to What exactly is core? for that.

Be strict about tests. The project has required tests for most changes to the Ruby code base, however we have not been historically as strict with UI changes. Ask for integration tests for any change that could break in future versions without tests. Testing is a powerful weapon to ensure regressions don't happen.

Be mindful of the use of the word 'you'. It can be easily misunderstood as you reviewing the 'person' not the 'code'. Try not to personalize any references to code. I.e: 'you changed that in line 42' vs 'that change in line 42'.

Back up your reviews by facts when they are not obvious or when contributors might not know why they should make some change. Adopt a "teaching" approach, but again, back up your 'teachings' with links to other sources.

Use '@' to raise the attention of the contributor about any issue. If an user is not 'watching' the project on GitHub or is not subscribed to a particular issue, GitHub will not show notifications nor send emails to that user.

If there is conflict, point to this handbook for reference.

Checklist

• Does it fix the problem described in the issue?
• Not fixing additional problems not described in the issue.
• Not missing string extractions (use :mark_translated: true).
• All string extractions follows our rules.
• Commit message follows the [format](Commit messages)
• Code follows the style rules mentioned above
• New Javascript files are added to config/environments/production.rb if not in app.js
• No new "stylesheet" tags are added to views, they're already in app.css

Labels

Labels help reviewers understand what is the status of a pull request. Thanks to them, reviewers can make sure no pull request remains unreviewed and when they get updated, they see it.

Not all our repositories use labels, so always make sure to comment after you update a pull request to notify the reviewer and explain the new changes.

• PR processor will set the labels Not reviewed and Needs testing when a pull request has just been submitted.
• After you have reviewed the pull request, mark it as Waiting on contributor if it needs changes or you're expecting answers.
• When the pull request branch gets updated, PR processor will automatically set the labels Needs re-review and Needs testing.
• Repeat the process until the code is ready to be merged.
• If there is a problem such as a serious disagreement between people about a change, or there's a dependency on other project that needs to be solved, set it as Reached an impasse.

Becoming a maintainer

As a maintainer, your responsibility is to keep a project running, taking care of certain parts of it. Examples of such parts are:

• A plugin, by taking care of documentation, code reviews, and packages to foreman-packaging
• Debian packaging, by looking at which projects need debian packages and updating them
• A subsystem, such as a Compute Resource or an Operating System in Foreman and updating it properly.

Traditionally, the process to become a maintainer has been simple. Take care of something for long enough and people will expect you to maintain it. We might ask you directly if you want to be a maintainer if you're maintaining something for some time. If you have made contributions only on a very specific topic (an Operating System, for instance), we might ask you to be a maintainer for that part of the code.

API usability and versioning

We follow Semantic versioning 2.0 for anything above '0.x' releases. This means incompatible changes, such as breaking an API endpoint, not working with a previous Foreman version (in the case of a plugin), require increasing the major version. The bulk of your changes should only require increasing the minor and patch versions, for backwards-compatible functionality and bugfixes.

We strive to remain compatible and not break other people's workflows, so if you have made some breaking changes that will need increasing the major version, please use Foreman::Deprecation to deprecate anything and warn in which version will it be removed. It supports two calls:

Foreman::Deprecation.api_deprecation_warning(message)

Foreman::Deprecation.deprecation_warning(foreman_version_deadline, message)

Architectural decisions

What exactly is core?

• Features that most of our userbase use. We get this information through surveys.
• Abstractions such as the Fact importer and parser, Power manager, etcetera.
• API
• A framework for plugin development.
• Compute Resources, Host groups, Reports, UI helpers, Nics, Host, Smart proxies are a few examples of key models in core.

What do we pull from core to make it a plugin?

• Features we do not want to maintain, either because they are complex and not widely used, or because they are difficult to test and maintain.
• Very specific features that most of our user base does not use.
• Examples of things we would consider moving to a plugin are: Host group based provisioning, GCE compute resource, APIv1.

Why some features might start as plugins?

The controls in core, such as deep code reviews, plus being subject to quarterly releases, make it difficult to quickly develop some features. For this reason, you might decide to start off your feature as a plugin. Then, once your code has been released, tested, and after review, it could be pulled into core if deemed appropriate.