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

Fixing logrotate issue in Makefile/Dockerfile #3233

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

jrynkiew
Copy link
Contributor

@jrynkiew jrynkiew commented Mar 25, 2022

Description

Dependency DOCKER_BUILDKIT=1 in Makefile.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (fix breaking change)

How Has This Been Tested?

  • I ran make docker

RUN chmod -R u=rwX,go=rX /etc in current Dockerfile

RUN chmod -R u=rwX,go=rX /etc
will cause the command DOCKER_BUILDKIT=0 docker build -t jeremi/iotex-core:latest . to compile successfully with no errors, but the build will fail if using BuildKit DOCKER_BUILDKIT=1 docker build -t jeremi/iotex-core:latest .

Since BuildKit is the recommended way to use Docker by Docker Support, as it is more optimized than regular docker build command without BuildKit, I propose to fix the issue by forcing the Makefile to use

docker:
	DOCKER_BUILDKIT=1 $(DOCKERCMD) build -t $(USER)/iotex-core:latest .

and removing the breaking change

RUN chmod -R u=rwX,go=rX /etc

from the Dockerfile. This command is not necessary, since the error it throws when using DOCKER_BUILDKIT=0 is not a fatal error, it is only an error in the display, like many others. The container complains about

Potentially dangerous mode on /etc/logrotate.d/iotex: 0664
error: Ignoring /etc/logrotate.d/iotex because it is writable by group or others.

but it is not a fatal error. The container will still build.

Thus my last commit was a breaking change. Sorry for the mistake, I did not test this previous fix with BuildKit, as I never used it before

I think the only way to avoid issues like these in the future is to build our own base image

@jrynkiew jrynkiew requested a review from a team as a code owner March 25, 2022 11:31
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #3233 (6abf8c7) into master (faf3c97) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3233   +/-   ##
=======================================
  Coverage   74.97%   74.97%           
=======================================
  Files         228      228           
  Lines       21367    21367           
=======================================
  Hits        16019    16019           
  Misses       4488     4488           
  Partials      860      860           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faf3c97...6abf8c7. Read the comment docs.

@dustinxie dustinxie linked an issue Mar 25, 2022 that may be closed by this pull request
@Liuhaai Liuhaai merged commit f85c9ad into iotexproject:master Mar 28, 2022
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.

docker build fail
4 participants