Skip to content

MINIFI-311 Move to alpine base for docker image.#118

Closed
ai-christianson wants to merge 1 commit intoapache:masterfrom
ai-christianson:MINIFI-311
Closed

MINIFI-311 Move to alpine base for docker image.#118
ai-christianson wants to merge 1 commit intoapache:masterfrom
ai-christianson:MINIFI-311

Conversation

@ai-christianson
Copy link
Contributor

This change takes the 1.5G ubuntu minifi image and reduces it to a 39MB image by using a multi-stage alpine build.

MINIFI-311 ported dockerfile to alpine
MINIFI-311 allow warnings on Linux for compatibility with Alpine
MINIFI-311 fixed spdlog path in BuildTests cmake file
MINIFI-349 moved to multi-stage docker build

@apiri
Copy link
Member

apiri commented Jul 27, 2017

reviewing

Copy link
Member

@apiri apiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @achristianson,

This is awesome and a very welcomed improvement. Functionally the build is all good and works like a charm. L&N considerations seem fine. I would like to request that we strip out those areas I commented on. In general, we've had some issues with included resources that we weren't using sometimes having uncertain origins. Could you please adjust those and then I think we should be good to go.

@@ -0,0 +1,90 @@
# Adapted from various sources, including:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove this from the source inclusion. Anything beyond the actual source should get stripped out to minimize LICENSE/NOTICE concerns.

@@ -0,0 +1,84 @@
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove the entirety of the bench subdirectory.

@@ -0,0 +1,49 @@
# *************************************************************************/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove the entirety of the example directory

@@ -0,0 +1,19 @@
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove the tests subdirectory and skip this part of the build for the lib

Copy link
Contributor

@phrocker phrocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. I agree with Aldrin's comments about the bench and test dirs.

@ai-christianson
Copy link
Contributor Author

Thanks for the feedback. I will make the required changes and update the PR.

MINIFI-311 ported dockerfile to alpine
MINIFI-311 allow warnings on Linux for compatibility with Alpine
MINIFI-311 fixed spdlog path in BuildTests cmake file
MINIFI-349 moved to multi-stage docker build
MINIFI-311 removed unneeded dirs from spdlog snapshot
@ai-christianson
Copy link
Contributor Author

@apiri removed those dirs & rebased. Should be good to go now.

@apiri
Copy link
Member

apiri commented Aug 2, 2017

reviewing changes

@apiri
Copy link
Member

apiri commented Aug 2, 2017

hey @achristianson,

everything looks good here. will get this merged in. thanks for your adjustments!

@asfgit asfgit closed this in 8389c8a Aug 2, 2017
nghiaxlee pushed a commit to nghiaxlee/nifi-minifi-cpp that referenced this pull request Jul 8, 2019
MINIFI-311 ported dockerfile to alpine
MINIFI-311 allow warnings on Linux for compatibility with Alpine
MINIFI-311 fixed spdlog path in BuildTests cmake file
MINIFI-349 moved to multi-stage docker build
MINIFI-311 removed unneeded dirs from spdlog snapshot
MINIFI-312 Handle spdlog compilation failures in Alpine Linux

This closes apache#118.

Signed-off-by: Aldrin Piri <aldrin@apache.org>
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.

3 participants