Note that, even if we're excluding the `node_modules/` folder from
version control, we aren't adding it to Capistrano's shared folders
because, when `node_modules` is a symbolic link, NPM removes it when
running `npm install`.
We're choosing version 18 because if offers support for SSL 3, just
like Ruby 3.1 does.
Note we're symlinking a .nvmrc file as well, in order to make it
compatible with NVM. While the .nvmrc and .node-version files use
different formats, they both support the syntax we're using to
define the version.
The code to install Node.js in the Dockerfile is a simplification of the
code in the Rails Dockerfile template [1].
[1] https://github.com/rails/rails/blob/04c97aec8a/railties/lib/rails/generators/rails/app/templates/Dockerfile.tt#L25
We originally set a daily interval because we hadn't updated our gem
dependencies for a year.
However, we usually wait a few days/weeks between the time a gem is
released and the moment we update it, and there are gems releasing new
versions every few days, so maintaining daily updates would become
tedious quickly.
So we're now doing it once a month. We're also increasing the limit of
open pull requests so we don't need to worry about whether dependabot is
opening pull requests for every dependency.
Since we changed the way we integrate coveralls in commit 8ed8cc8b9,
we're getting 6 additional checks displayed in our pull requests.
We don't need these checks, and they only add noise. The only reason we
use coveralls is to know the test coverage in our master branch.
So we're changing the code so coveralls only runs on the master branch.
There's also a chance that the test suite will be faster because it
doesn't need to keep track of the coverage, although I haven't noticed any
significant differences during my tests.
I haven't found a more elegant way to say that a certain step should
only be run on push on master, so I'm setting the environment variable
we were already using.
Coveralls stopped working back in July when we reached build number 3790
because back when we used Travis we created builds from numbers 3791 to
35700.
After trying to change the numbers in several ways, all of them
resulting in a "No build matching CI build number" message, we're trying
the Github Action provided by Coveralls instead. In order to make it
work, we need to add the `simplecov-lcov` gem and generate the report at
`coverage/lcov.info`. Note that, for some reason, `simplecov-lcov`
doesn't seem to depend on `simplecov` and we need `simplecov` 0.18 or
later, so we're manually adding this dependency as well.
In the past, we've run into situations where we commited a version of
the schema.rb to version control which wasn't in sync with what you get
when you create the database and run the migrations. This is something
that might happen if you're working on different branches at the same
time, run the migrations in all those branches, and then commit the
current status of your database to your current branch. That will result
in a schema file which contains the database changes done in all these
branches instead of just the current one.
Although this hasn't happened to us for years, we see it happening
sometimes in other COSUL installations, and there's always a chance that
we make a mistake and do it again.
Until now, it wasn't a big deal because the installer sets up the
database by running `db:create db:migrate`, which runs the migrations
instead of using the `schema.rb` file.
However, we're going to add multitenancy using the Aparment gem, and
this gem uses the `schema.rb` file when creating a new schema for a new
tenant [1]. If the `schema.rb` file contains changes that shouldn't be
there, this will lead to different database schemas having different
tables and/or columns and future migrations might crash on production
when applied to some of these schemas. In other words, this mistake
could now result in a disastrous situation.
To help preventing that, we're adding a CI workflow that checks the
current `schema.rb` file is in sync with the database migrations.
[1] https://medium.com/infinite-monkeys/our-multi-tenancy-journey-with-postgres-schemas-and-apartment-6ecda151a21f#7e97
We've been doing manually for too long ;).
The reason why we're assigning the author is it makes it easy to filter
pull requests by assignee on our kanban; it isn't so easy (actually,
might be impossible) to filter by author.
Without this condition, the kanban assignment would be run on every
CONSUL fork and that would result in unpredictable results since they
would try to write on our kanban, and they don't have permission to do
so.
Note that, much to my dismay :D, the code only works if we use single
quotes in the name of the repository owner; it doesn't work if we use
double quotes.
So now, newly opened pull requests will automatically be added to the
"Reviewing" column, while newly opened draft pull requests will
automatically be added to the "Doing" column.
I've added the "reopened" event just in case (and was indeed very useful
while testing this feature), although we rarely reopen pull requests.
Note that this only works on pull requests that aren't already in the
project; that is, if a pull request is already in the "Doing" column,
closing it and reopening it will *not* move it to the "Reviewing
column".
So it looks like we won't easily be able to extend this feature in order
to automatically move pull requests when they're marked as ready for
review.
Since the target branch was in a different repository, the action failed
since it couldn't find the reference.
The code here is based on a recent change in Pronto [1] and with a comparison
between the repo.url property of pull_request.head and pull_request.base
to determine if the pull request was created from a forked repository
[1] https://github.com/prontolabs/pronto/commit/4fe28418b6
Sometimes a test gets stuck and and we have to wait until it times out
in order to check which files were being tested at the time.
The default timeout is six hours. I'm reducing it to one hour which
should still be plenty of time even on repositories with no knapsack
token.
Somehow I thought it worked automatically, but we had to provide the
token.
The configuration is based on Coveralls instructions to run parallel
builds [1].
Alternatively we could use the Coveralls GitHub Action [2] which
slightly simplifies the workflow configuration and removes the
dependency of the coveralls gem. However, it also adds a dependency on
simplecov-lcov and requires configuring it to renerate LCOV files on
each run, so the benefits of using it are not that big.
[1] https://docs.coveralls.io/parallel-build-webhook
[2] https://github.com/coverallsapp/github-action/
Due to a bug in dependabot (see issue 2198 in the
`dependabot/dependabot-core` repository), Dependabot would open pull
requests on every fork, which would be inconvenient.
So we're disabling the updates until this issue is fixed; since we
currently have almost 50 open pull requests, it shouldn't affect us that
much.
These lines were added when we were trying the pronto-eslint_npm gem,
which isn't maintained anymore and is not compatible with the latest
Pronto version.
Since we're now using the pronto-eslint gem, and this gem depends on the
eslintrb gem (which includes ESLint), we don't need these lines anymore.
This way developers can run the checks on their machines and using
`bundle exec` we guarantee the right versions of all our gems are being
used; with Hound we can only use the versions supported by their
service.
When including the pronto-erb_lint gem, we're getting errors in
development where our ERB does not follow the conventions Better HTML
expects. Since we only use Better HTML because ERB Lint depends on it,
and right now we are not ready to follow its conventions, we're
disabling it.
Note pronto depends on rugged, which requires CMake and pkg-config to
build the `libgit2` library it depends on. CMake and pkg-config are
installed by default in some GNU/Linux distributions like Ubuntu, but
might not be installed on other systems, so we're adding them as
development dependencies.
Using GitHub Actions has a few advantages over using Travis CI:
* More jobs can be run in parallel
* All CONSUL repositories on GitHub will be configured automatically
Besides, Travis have recently changed their policy twice. First, they
announced their site for free software projects would be shut down but
free software projects could still use their site for private projects.
And then, they limited the usage of their services for free software
projects.
Just like we used to do with Travis, we're enabling builds for pull
requests but not for pushed branches.
We're also building the master branch. Even if we never push to the
master branch directly, we're aware other CONSUL repositories do, so
we're running the tests for this case.
The actual PR template feels more like bureaucracy than an actual guide
or checklist to help the PR author explain all important thigs that any
reviewer or changelog reader may need to understand.
We'll be moving most of the redundant things (like remembering tests are
needed, or explaning how things where implemented with a clear and
granular commit history) into a Wiki/Doc entry.
For regular contributors there is no need for reminders, we need to
improve new contributors landing with good guides and lowering the bar
for first PR's
Why:
* The template tried to specify a placeholder for the title, but instead it was used as first line in the description
* The template tried to specify a guide for each section with a `>` character to imply it was a comment, but people didn't remove it
How:
* Removing the first line specifying the title placeholder that has no use
* Changing the `>` character on each section guide for a `-` that can be easily reused for enumerations
