-
Notifications
You must be signed in to change notification settings - Fork 153
CI improvements #159
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
CI improvements #159
Conversation
b9d74d6 to
fdb38f2
Compare
* Add Elixir 1.12 and OTP 24 to the build matrix. * Drop Elixir 1.8 and OTP 21.
The previous setup was actually skipping all tests due to a conflict between `--only integration` and `exclude: [integration: true]`. Now we are running all tests except for the integration ones, which will be added back next.
There are currently only 4 of them and they will go green regardless of the protoc run success. Promoting it to an actual CI step now will make it easier to spot failures.
| echo "./bin" >> $GITHUB_PATH | ||
| - name: Compile .proto files to Elixir with protoc | ||
| if: ${{matrix.integration}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we should actually flag it like this or move it to a separate job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great 👍
| - mix test.integration | ||
| - mix hex.build | ||
| # Only run for latest version | ||
| - '[[ "$TRAVIS_ELIXIR_VERSION" =~ 1.[8-9] ]] || mix format --check-formatted' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we don't check formatting in the new pipeline anymore. Should we bring it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it yes, but in a later PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@britto actually can we also add mix compile --warnings-as-errors and mix deps.unlock --check-unused to the lint: true CI step as part of linting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll give it a shot.
| if: ${{matrix.integration}} | ||
| run: | | ||
| wget https://github.com/protocolbuffers/protobuf/releases/download/v3.12.3/protoc-3.12.3-linux-x86_64.zip | ||
| unzip protoc-3.12.3-linux-x86_64.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it doesn't take too long, it would be ideal if we could cache this. Maybe an improvement for the next iteration.
whatyouhide
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely 💯
Also make sure the tests actually run in CI. The previous setup was skipping all the tests due to a combination of
--only integrationin the command line andexclude: [integration: true]intest_helper.exs. Now we are running all regular tests in a step:Then the 4 integration ones in another step, after we install
protocand make sure it compiles a few files correctly, as before: