-
Notifications
You must be signed in to change notification settings - Fork 344
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
Update development dockerfiles to use prebuilt extensions #2673
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goodness, this is some serious cleanup - very nice!
I just had one comment about capturing the version.
Makefile
Outdated
elyra-image-dev: | ||
@mkdir -p build/dev/docker | ||
cp etc/docker/elyra/Dockerfile.dev build/dev/docker/Dockerfile | ||
cp etc/docker/elyra/start-elyra.sh build/dev/docker/start-elyra.sh | ||
cp etc/docker/elyra/requirements.txt build/dev/docker/requirements.txt | ||
cp dist/elyra-$(ELYRA_VERSION)-py3-none-any.whl build/dev/docker/ | ||
docker buildx build \ | ||
--progress=plain \ | ||
--output=type=docker \ | ||
--tag docker.io/$(ELYRA_IMAGE) \ | ||
--tag quay.io/$(ELYRA_IMAGE) \ | ||
--build-arg TAG=$(ELYRA_VERSION) \ | ||
build/dev/docker/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This probably isn't a big deal but thought I'd mention it.)
With these dev targets, if either of the -image-dev
targets are invoked when the ELYRA_VERSION
is not a .devN
version (e.g., 3.8.0
), then the image produced by these targets will overwrite the non-dev images. Not sure if its worth adding some kind of detection, like confirm ELYRA_VERSION
contains ".dev"
, prior to proceeding. Since these targets are referenced by container-images-dev
and not container-images
it probably isn't something to be concerned about. I also haven't looked closely into the differences between the dev and non-dev dockerfiles used by these targets as it could be the case that, when using a released wheel file, the two are essentially equivalent images.
Makefile
Outdated
elyra-image-dev: | ||
@mkdir -p build/dev/docker | ||
cp etc/docker/elyra/Dockerfile.dev build/dev/docker/Dockerfile | ||
cp etc/docker/elyra/start-elyra.sh build/dev/docker/start-elyra.sh | ||
cp etc/docker/elyra/requirements.txt build/dev/docker/requirements.txt | ||
cp dist/elyra-$(ELYRA_VERSION)-py3-none-any.whl build/dev/docker/ | ||
docker buildx build \ | ||
--progress=plain \ | ||
--output=type=docker \ | ||
--tag docker.io/$(ELYRA_IMAGE) \ | ||
--tag quay.io/$(ELYRA_IMAGE) \ | ||
--build-arg TAG=$(ELYRA_VERSION) \ | ||
build/dev/docker/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This probably isn't a big deal but thought I'd mention it.)
With these dev targets, if either of the -image-dev
targets are invoked when the ELYRA_VERSION
is not a .devN
version (e.g., 3.8.0
), then the image produced by these targets will overwrite the non-dev images. Not sure if its worth adding some kind of detection, like confirm ELYRA_VERSION
contains ".dev"
, prior to proceeding. Since these targets are referenced by container-images-dev
and not container-images
it probably isn't something to be concerned about. I also haven't looked closely into the differences between the dev and non-dev dockerfiles used by these targets as it could be the case that, when using a released wheel file, the two are essentially equivalent images.
Is there a way to use this effort to try to clean up vs adding additional duplication? Now that we are supposedly building proper pre-built images that can be installed, can't we identify the tag name with something like this and remove the duplicated makefile tasks and dockerfile? |
Thanks, @akchinSTC, this looks a lot cleaner. I can give it a try on the functionality over the weekend but otherwise looks good. |
Adds a new
container-image-dev
target to MakefileModifies existing dev dockerfiles to use pre built and packaged extensions.
Fixes #2546
What changes were proposed in this pull request?
How was this pull request tested?
New make target was run and container images and tags verified.
existing non dev targets should remain unaffected and should continue to function during release process.
Developer's Certificate of Origin 1.1