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

Create Dockerfile #499

Closed
wants to merge 5 commits into from
Closed

Create Dockerfile #499

wants to merge 5 commits into from

Conversation

guitcastro
Copy link
Contributor

What is the purpose of the pull request

Create a docker file to run the project

Brief change log

  • Create Dockerfike

Verify this pull request

This pull request is a trivial rework / code cleanup without any test coverage.

Copy link
Contributor

@the-other-tim-brown the-other-tim-brown left a comment

Choose a reason for hiding this comment

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

Should we add some readme about how to use this dockerfile?


ENTRYPOINT [
"java", \
"--add-opens=java.base/sun.nio.hb=ALL-UNNAMED", \
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment explain why we want these flags?

Copy link
Contributor Author

@guitcastro guitcastro Jul 31, 2024

Choose a reason for hiding this comment

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

It's necessary to added this flag to run spark on java >= 17.

Sorry, I forgot to send the link

USER appuser

# Copy the executable from the "package" stage.
COPY --from=package build/target/app.jar ./app.jar
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that only the jar will be included in the final image and not all the classes? I have not used a multi-stage build before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly!

@guitcastro
Copy link
Contributor Author

guitcastro commented Jul 31, 2024

Should we add some readme about how to use this dockerfile?

Yes, that's how I am using, but you probably should deploy the image somewhere. I could create a PR with a workflow to publish on github container register, if you agree.

docker run \
  -v ./xtable/config.yml:/xtable/config.yml \
  -v ./xtable/core-site.xml:/xtable/core-site.xml \
  -v ./xtable/catalog.yml:/xtable/catalog.yml \
  registry.gitlab.com/pedalin/xtable-docker-image:4420d2b6 \
  --datasetConfig /xtable/config.yml --hadoopConfig /xtable/core-site.xml --icebergCatalogConfig xtable/catalog.yml

@the-other-tim-brown
Copy link
Contributor

the-other-tim-brown commented Aug 1, 2024

Should we add some readme about how to use this dockerfile?

Yes, that's how I am using, but you probably should deploy the image somewhere. I could create a PR with a workflow to publish on github container register, if you agree.

docker run \
  -v ./xtable/config.yml:/xtable/config.yml \
  -v ./xtable/core-site.xml:/xtable/core-site.xml \
  -v ./xtable/catalog.yml:/xtable/catalog.yml \
  registry.gitlab.com/pedalin/xtable-docker-image:4420d2b6 \
  --datasetConfig /xtable/config.yml --hadoopConfig /xtable/core-site.xml --icebergCatalogConfig xtable/catalog.yml

@guitcastro can you update the README with this as an option for building and running? I am imagining users that are not setup with java locally could benefit from this.

I will check where we can publish the images, we are currently doing our first release as part of the incubator program

@guitcastro
Copy link
Contributor Author

@the-other-tim-brown Right, I will update the Readme!

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@the-other-tim-brown the-other-tim-brown left a comment

Choose a reason for hiding this comment

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

@guitcastro can you squash this down to a single commit for me after adding the license? Our current setup doesn't allow us to do squash and merge from GitHub so just need things to be a single, cleaned up commit before I merge it

@@ -0,0 +1,42 @@
# syntax=docker/dockerfile:1
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to add the apache license in the comments here to make CI pass

added apache license
@guitcastro guitcastro mentioned this pull request Aug 7, 2024
@guitcastro
Copy link
Contributor Author

Continue on #506

@guitcastro guitcastro closed this Aug 7, 2024
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