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

Supports Alpine by detecting OS using /etc/alpine-release #934

Merged
merged 3 commits into from
Oct 4, 2020
Merged

Supports Alpine by detecting OS using /etc/alpine-release #934

merged 3 commits into from
Oct 4, 2020

Conversation

codefromthecrypt
Copy link
Contributor

Summary

Alpine is a small OS distribution that supports JDK. Supporting Alpine
means less download time and cost when building projects in Docker.

This solves the same problem #853 did, via a different approach.

  • Move the default download root to the Platform impl
  • Allow a classifier (currently only musl, but there are others)
  • Special case Alpine instead of generically

The above changes allow someone to override the download root regardless
of if the nodejs dist is experimental or not.

Tests and Documentation

This backfills tests for Platform using the same approach as we do in Brave
(powermock).

This obviates #853 via a slightly different approach

* Move the default download root to the `Platform` impl
* Allow a classifier (currently only musl, but there are others)
* Special case Alpine instead of generically

The above changes allow someone to override the download root regardless
of if the nodejs dist is experimental or not.

Finally, this backfills tests for `Platform` using the same approach as
we do in Brave (powermock).
codefromthecrypt pushed a commit to openzipkin/zipkin that referenced this pull request Oct 3, 2020
@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Oct 3, 2020

testing manually here (our builder image is alpine) https://github.com/openzipkin/zipkin/compare/DO_NOT_MERGE_test-frontend-maven-plugin

@codefromthecrypt
Copy link
Contributor Author

^^ works fine!

@eirslett
Copy link
Owner

eirslett commented Oct 4, 2020

OK, let's go with this implementation!

@eirslett eirslett merged commit 6688cfd into eirslett:master Oct 4, 2020
@eirslett
Copy link
Owner

eirslett commented Oct 4, 2020

There we go, it's in the 1.10.2 release.

@codefromthecrypt
Copy link
Contributor Author

Thanks for the quick turnaround, @eirslett!

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.

2 participants