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

Corrections to get build checks passed #386

Merged
merged 2 commits into from
Apr 20, 2020

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Apr 15, 2020

Pin version of test dependency snabbkaffe

This avoids breakage due to test framework development,
and will enable the common test suites again.

Update test to please DRY rule check in elvis

This corrects an issue found during the Travis build, from #385

When Travis is running it uses the latest version of rebar3 and elvis,
but the latest elvis has a change regarding how it handles macros.
Elvis depends on katana_code, which has replaced the pre-processor handling, see:
inaka/katana-code#31

This has changed the behavior of elvis to also include code defined in TEST scope in its checks,
which this PR simply fixes.
This could also be solved by pinning elvis to a specific older version..

@mikpe
Copy link
Contributor

mikpe commented Apr 15, 2020

It would be nice if someone could fix the elvis breakage...

@k32
Copy link

k32 commented Apr 18, 2020

@mikpe As a repo owner you can force merge this PR, then deal with elvis checks separately

@k32
Copy link

k32 commented Apr 19, 2020

Thanks for the PR! Alternatively, this dependency can be fetched from hex: https://hex.pm/packages/snabbkaffe

@bjosv
Copy link
Contributor Author

bjosv commented Apr 19, 2020

Ah thanks, I didn't want to change the source in case there was a reason for it.
Maybe there can be a commit changing all deps to hex if we want that?
I guess I cant help getting this merged unless the #385 is merged first?..and vice versa

@k32
Copy link

k32 commented Apr 19, 2020

Hi!

Ah thanks, I didn't want to change the source in case there was a reason for it.

I don't think there is a good reason to keep this dependency as a source. If I remember correctly, this package hadn't been published to Hex at the time.

I guess I cant help getting this merged unless the #385 is merged first?..and vice versa

I guess both PRs are needed to get a green build. Order should not be important.

This avoids breakage due to test framework development.
The latest elvis is using a new version of katana_code,
which has replaced the pre-processor handling, see:
inaka/katana-code#31

This has changed the behavior of elvis to also check code
defined in TEST scope.
@bjosv bjosv changed the title Pin version of test dependency snabbkaffe Corrections to get build checks passed Apr 20, 2020
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1365

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 80.325%

Files with Coverage Reduction New Missed Lines %
src/brod_client.erl 1 85.45%
Totals Coverage Status
Change from base Build 1353: 0.07%
Covered Lines: 2225
Relevant Lines: 2770

💛 - Coveralls

@bjosv
Copy link
Contributor Author

bjosv commented Apr 20, 2020

I don't think there is a good reason to keep this dependency as a source. If I remember correctly, this package hadn't been published to Hex at the time.

Sound reasonable and I changed this..much better.

I guess both PRs are needed to get a green build. Order should not be important.

Since its was two small PRs I merged them into this, seems that I got a coverage run now atleast.
Hope it makes it easier and not only confusing :)

@k32
Copy link

k32 commented Apr 20, 2020

Thank you!

@jesperes jesperes merged commit f76c456 into kafka4beam:master Apr 20, 2020
@bjosv bjosv deleted the pin_snabbkaffe_version branch April 21, 2020 07:39
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.

5 participants