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

chore: add docker-build target to Makefile with support for configurable build flags #4893

Merged
merged 9 commits into from
Nov 7, 2024

Conversation

gacevicljubisa
Copy link
Contributor

@gacevicljubisa gacevicljubisa commented Nov 4, 2024

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

This PR introduces a new target, docker-build, in the Makefile, allowing for configurable build flags when creating the Docker image. Specifically, users can now set the REACHABILITY_OVERRIDE_PUBLIC and IMAGE flags, along with other flags as needed, when building the image.

Example of usage:

make docker-build REACHABILITY_OVERRIDE_PUBLIC=true IMAGE=ethersphere/bee:latest

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

@gacevicljubisa
Copy link
Contributor Author

Way to use build flag when creating docker image:

docker build --build-arg REACHABILITY_OVERRIDE_PUBLIC=true -t bee-image-name -f Dockerfile .

@acha-bill
Copy link
Contributor

If we needed to support other flags defined in the makefile here as well, we would end up maintaining variables in 2 places.
Would it work if we exposed the variable in the makefile to the environment so that the value can be passed to make?

makefile

export REACHABILITY_OVERRIDE_PUBLIC ?= false

Then use it with the build arg as you suggested

@gacevicljubisa
Copy link
Contributor Author

gacevicljubisa commented Nov 5, 2024

If we needed to support other flags defined in the makefile here as well, we would end up maintaining variables in 2 places. Would it work if we exposed the variable in the makefile to the environment so that the value can be passed to make?

makefile

export REACHABILITY_OVERRIDE_PUBLIC ?= false

Then use it with the build arg as you suggested

we could add this to Makefile:

# Load .env if it exists
ifneq (,$(wildcard ./.env))
	include .env
	export
endif

And if we locally have .env file with the specific flag set to the value. Then no other changes or maintinance is needed.

Another way would be that we have a new target in Makefile that will be responsibile for building the image itself:

docker-build:
	docker build -t $(IMAGE) . --no-cache

And this is example how it can be used:

make docker-build REACHABILITY_OVERRIDE_PUBLIC=true IMAGE=ethersphere/bee:latest

@nugaon
Copy link
Member

nugaon commented Nov 5, 2024

Another way would be that we have a new target in Makefile that will be responsibile for building the image itself:

I would vote on this option.

having .env variable is error prone between builds.

@gacevicljubisa gacevicljubisa marked this pull request as ready for review November 5, 2024 10:24
Copy link
Contributor

@acha-bill acha-bill left a comment

Choose a reason for hiding this comment

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

lgtm.
Please rename the PR title

@gacevicljubisa gacevicljubisa changed the title chore: set reachabilityOverridePublic build flag in docker image chore: add docker-build target to Makefile with support for configurable build flags Nov 5, 2024
@gacevicljubisa gacevicljubisa added the in progress ongoing development , hold out with review label Nov 5, 2024
@gacevicljubisa
Copy link
Contributor Author

gacevicljubisa commented Nov 5, 2024

Another way would be that we have a new target in Makefile that will be responsibile for building the image itself:

I would vote on this option.

having .env variable is error prone between builds.

@acha-bill @nugaon
Proposed solution will not work without .env file.

The only idea I have now is back to docker build arguments.

@darkobas2 do you have any other better idea?
The con is that we currently need to maintain variables in 2 places.

@gacevicljubisa gacevicljubisa removed the in progress ongoing development , hold out with review label Nov 5, 2024
@darkobas2
Copy link
Contributor

darkobas2 commented Nov 5, 2024

Maybe instead of separate image i would suggest to just build and add an additional binary into the image.... And then it can be chosen at will

@gacevicljubisa
Copy link
Contributor Author

gacevicljubisa commented Nov 6, 2024

I discovered an issue when deploying the cluster locally using beelocal. Locally, it always uses the default values defined in the Makefile, whereas on CI, it works as expected, passing the values correctly.

To address this issue and allow building the image with REACHABILITY_OVERRIDE_PUBLIC and BATCHFACTOR_OVERRIDE_PUBLIC, I made the PR in beelocal repo.

Additionally, the old Dockerfile has been left intact so that users can still build the image in the previous way if needed.

These updates should ensure that local deployments work the same way as on CI.

Default Values:

  • REACHABILITY_OVERRIDE_PUBLIC: false (default)
  • BATCHFACTOR_OVERRIDE_PUBLIC: 5 (default)
  • BEE_IMAGE: ethersphere/bee:latest (default Docker image tag)

How to Use:

  1. To build the Docker image using default values: Simply run:

    make docker-build
  2. To override the default values:

    make docker-build REACHABILITY_OVERRIDE_PUBLIC=true BATCHFACTOR_OVERRIDE_PUBLIC=2 BEE_IMAGE=ethersphere/bee:v1.0

Copy link
Contributor

@acha-bill acha-bill left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@nugaon nugaon left a comment

Choose a reason for hiding this comment

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

LGTM!

@gacevicljubisa gacevicljubisa merged commit be17cd2 into master Nov 7, 2024
14 checks passed
@gacevicljubisa gacevicljubisa deleted the dockerfile-args branch November 7, 2024 14:21
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