Skip to content

Latest commit

 

History

History
411 lines (273 loc) · 28.1 KB

handbook.md

File metadata and controls

411 lines (273 loc) · 28.1 KB
Raw
layout title
contribute
Handbook

How we make things

Branching and SCM

We use git for source control. The core projects:

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".

If the commit that you are adding fixes multiple, distinct issues you can follow this example:

Fixes #9998,#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.

Notes

  • Some background to justify this commit message style can be found here.
  • More tips for writing better commit messages 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. Update the Error Codes wiki 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

JavaScript

We are supporting and encouraging the use of es2015. (For more information see Learn ES2015.) For browser compatibility, we use Babel to transform the code to es5.

We use eslint to enforce linting rules. Run eslint by typing npm run lint in the command line at the root directory.

Do

  • Add all new JavaScript files to the project in the 'webpack/assets/javascripts' folder or a sub-folder.
  • Use es2015 syntax and features.
  • Use exporting/importing values from/to modules to manage dependencies.

Don't

  • Pollute the global namespace. If, for some reason, you must expose a function/value globally, use the window.tfm object as implemented in bundle.js.
  • Remove global functions from existing code without deprecating them first. Deprecation can be done by calling the tfm.tools.deprecate function.

Shared modules

Usually, when we add a new node-module, we want it to be shared with plugins (Unless we have a reason not to share it).

In order to do so, we let webpack create a vendor.js file with the relevant modules. We manage the shared-modules list in config/webpack.vendor.js, when adding/removing node-modules, need to update this file accordingly.

Linting rules

The latest lint rules for foreman can be found here. See ESLint Rules for more information regarding rule configuration.

It is possible to disable a specific rule temporarily. You should have a good reason for doing this. For more information see Configuring ESLint.

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 collectible)
  • 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 a 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

Time zones

All date and time information should be saved in UTC to the database, for consistency. Similarly, all date and time information shown to users must either take into account the user time zone (accessible through User.current.timezone) or not specify anything. Foreman's controllers will call 'set_timezone' upon every request, which is a method that will automatically set a time zone and modify Time objects in Ruby to use it.

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 the Development board on our forum. 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.

Becoming a committer

The following process applies to the core projects mentioned at the beginning (Foreman, Smart-Proxy, Foreman-installer and Foreman-SElinux). Most plugins follow a more informal process.

What is a committer

A Foreman committer is a person with commit access to one or more of the repositories under the Foreman organization. Committers are members of the Foreman contributors community who exhibit most of the following behaviors:

  • Review and merge code and documentation
  • Help triaging bugs and testing pull requests
  • Make well formed pull requests
  • Take care of releases
  • Have a sense of duty about the Foreman project
  • Play well with others, are respectful, show gratitude

We ask new Foreman contributors to 'act as a committer' to the extent we feel they are capable.

If you want to become a committer, we expect you help with some of these tasks:

  • Review and test the patches submitted by others; this helps to offload the burden on existing committers, who will definitely appreciate your efforts
  • Participate in discussions about releases, road maps, architecture, and long-term plans
  • Improve the website
  • Improve project infrastructure in order to increase the efficiency of committers and other contributors
  • Help raise the project's quality bar (e.g. by setting up code coverage analysis)
  • As much as possible, keep your activity sustained rather than sporadic

Other things that are nice to do:

  • Support users and other developers on the forum, in Redmine, and in IRC #theforeman and #theforeman-dev
  • Participate in (or even initiate) real-world events such as user/developer meetups, papers/talks at conferences, etc

How do I become a committer?

One person has to nominate you to the group of existing committers. The person who nominates you has to:

  • Submit 10 examples that prove this person behaves like a committer. For instance:
    • Strategic patches to the project that fix or add key functionality
    • Forum topics where the person is clearly helpful
    • Comments that prove the person has triaged bugs
    • Proof the nominee has maintained a plugin successfully
    • Anything else that shows the nominee should become a committer
  • Explain how the nominee is involved in the community and cares about the future of the project

This nomination is public and should be made to the Development forum board. After the nomination is submitted, two other committers have to second the nomination. If no one objects in one week, the nomination is accepted.

Such objections may happen in public on the nomination thread. Understandably, however, not everyone is comfortable giving objections publicly. Therefore, it is acceptable for other committers to raise their concerns with the sponsor and/or other committers privately if they wish to do so. The sponsor is expected to update the nomination thread to show that it is on hold pending private concerns.

Regardless, during any objections, private or public, the nomination is on hold until the objections are resolved or the nomination is rejected. In the event of a failed nomination, the sponsor (as part of the discussing group) will know the grounds for the rejection, and can pass along constructive feedback to the candidate. Care should be taken to do this sensitively.

How do I lose committer status?

If you are inactive in the community for one year, we will remove you from the committers list and revoke your permission, but we will make a mention of you in theforeman.org list of previous committers.

In the event that a committer continues to disregard good citizenship (or actively disrupts the project), we may need to revoke that person's status. The process is the same as for nominating a new committer: someone suggests the revocation with a good reason, two people second the motion, and a vote may be called if consensus cannot be reached. We hope that's simple enough, and that we never have to test it in practice.

Special thanks to Chromium, Mozilla, V8, FreeBSD and Apache Hive for providing the basis of this procedure.

Quick tips for new committers

  • If something you merged broke something, the build, etc… you take responsibility
  • Do not merge your own commits unless the other committers in the group accept this behavior (usual in small plugins)
  • Do not merge commits blindly. If you do not fully understand a pull request, ask existing committers to take a look
  • Do not merge if the build is failing. Wait until tests are green to merge.

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.

Technical notes

Using the Spring preloader in development

The Spring preloader is installed and enabled by default for development installations to help run Rails consoles and tests quickly.

What you need to know:

  1. Running bin/rake test test/unit/example_test.rb, bin/rails c and similar commands will automatically start and use Spring. Note the bin/ prefix!

  2. Spring runs background processes to speed up commands: use spring status to see them and spring stop to stop (i.e. restart) them. They should automatically restart when required, but improve config/spring.rb if necessary.

Other useful information:

  1. bin/rake test test/unit/example_test.rb will also run all installed plugin tests, use ruby -Itest test/unit/example_test.rb as a workaround.

  2. export DISABLE_SPRING=true if you don't want to use it at all.

  3. To add bin/ to your path automatically so the bin/ prefix isn't required:

    1. Install direnv and enable it in your shell rc file (Fedora packages)
    2. echo PATH_add bin > .envrc in Foreman's dir

  4. See the Spring README for more useful information.

Using the webpack development server

To allow live reload of front-end assets, Foreman includes a small node.js server provided by webpack. In order to run foreman in development with the webpack development server, you have two options:

  1. foreman start will start both servers simultaneously. Note that using pry for debugging in this option is limited, as foreman does not echo back your input. You can choose to run a different rails server then the default (which is bin/rails server) by setting the RAILS_STARTUP environment variable.
  2. In separate terminals, run bin/rails server and ./node_modules/.bin/webpack-dev-server --config config/webpack.config.js. This will allow you to run pry inside the rails application as usual.

If you do not want to use the webpack development server (for example, if you only work on the backend), you can set :webpack_dev_server: false in config/settings.yaml which will disable it. You can then proceed to running foreman as usual (e.g. bin/rails s). In this case, however, you will need to manually run rake webpack:compile to generate the webpack managed assets.

Please note that bin/rails server is bound to localhost by default. Should you need to change the binding (e.g., to a specific ip address or to 0.0.0.0), please use either BIND=:: foreman start or create a .env file with BIND=::