Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add recommended combinations of Elixir and Erlang/OTP versions to CI #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adrianomitre
Copy link

@adrianomitre adrianomitre commented Oct 16, 2020

This pull request makes sure all recommended Elixir and Erlang/OTP combinations are run on CI.

Quoting José Valim:

I would still advise folks to go with:

  • For each Elixir version, add a run with earliest supported Erlang/OTP
  • For the last Elixir version, also test the latest supported Erlang/OTP

Please refer to the original discussion on dashbitco/broadway#188 for the full rationale.

Additional functionality: make sure there are no compilation warnings, both on application and on tests, and checks formatting.

Finally, solved some minor unused variable warnings, courtesy of improved checks of the Elixir 1.11.0 compiler, and credo strict minor stylistic issues.

In case one wonders why not use otp: N.x for OTP as well as for Elixir, please refer to actions/setup-elixir#45.

@adrianomitre adrianomitre force-pushed the add_recommended_combinations_of_elixir_and_erlang_versions_to_ci branch 10 times, most recently from 883204e to 4d80f09 Compare October 18, 2020 01:48
@adrianomitre adrianomitre force-pushed the add_recommended_combinations_of_elixir_and_erlang_versions_to_ci branch from ffa0272 to de13ec0 Compare October 29, 2020 00:31
@adrianomitre
Copy link
Author

@keathley @garthk @Qqwy @wojtekmach I am looking forward to hearing your thoughts on this pull request.

@adrianomitre adrianomitre force-pushed the add_recommended_combinations_of_elixir_and_erlang_versions_to_ci branch from de13ec0 to 8a69747 Compare October 29, 2020 00:41
@adrianomitre adrianomitre force-pushed the add_recommended_combinations_of_elixir_and_erlang_versions_to_ci branch from 8a69747 to 7e4293e Compare October 29, 2020 03:47
@wojtekmach
Copy link
Contributor

As I believe I’ve already stated elsewhere, Im not a fan of dense CI build matrices by default, I’d only reserve them for projects that would really benefit from them (eg elixirls) and for all the others, I’d test against just oldest and just newest supported version. This way we minimize resource usage that was given to us free of charge.

Regarding warnings as errors in tests, the best solution I found is the following:

# test/test_helper.exs
if System.get_env(“CI”) do
  Code.compiler_options(warnings_as_errors: true)
end

This isn’t obvious and I should have an article about it next week or so :)

@garthk
Copy link
Contributor

garthk commented Oct 29, 2020

Oh, handy.

I've been struggling with the full CI matrix on some other projects. I wish I'd thought of running just the oldest Elixir on the oldest OTP and the youngest Elixir on the youngest OTP. It'll deliver most of the benefit in a fraction of the resources. Thanks!

@Qqwy
Copy link
Contributor

Qqwy commented Oct 29, 2020

As I believe I’ve already stated elsewhere, Im not a fan of dense CI build matrices by default, I’d only reserve them for projects that would really benefit from them (eg elixirls) and for all the others, I’d test against just oldest and just newest supported version. This way we minimize resource usage that was given to us free of charge.

Agreed, with one little caveat (this has bitten me in the past):

Sometimes it might be worthwhile to also test newest Elixir with one-but-newest OTP and one-but-newest Elixir with the newest OTP. Sometimes this interplay will result in (compilation) errors you might otherwise miss.

adrianomitre pushed a commit to adrianomitre/otp_version that referenced this pull request Nov 3, 2020
adrianomitre pushed a commit to adrianomitre/otp_version that referenced this pull request Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants