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

[docker] build files for automated builds #836

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
3 participants
@jimschubert
Member

jimschubert commented Aug 17, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Creates multi-stage docker files for use on Docker Hub for automated builds at https://hub.docker.com/r/openapitools/openapi-generator.

@jimschubert

This comment has been minimized.

Show comment
Hide comment
@jimschubert

jimschubert Aug 17, 2018

Member

@wing328 @jmini if this gets merged into master, I can setup automated builds using patterns for branches, tags, and master. I could also look at configuring https://hub.docker.com/r/openapitools/openapi-generator-cli and https://hub.docker.com/r/openapitools/openapi-generator-online to work as triggered builds for releases as well.

Member

jimschubert commented Aug 17, 2018

@wing328 @jmini if this gets merged into master, I can setup automated builds using patterns for branches, tags, and master. I could also look at configuring https://hub.docker.com/r/openapitools/openapi-generator-cli and https://hub.docker.com/r/openapitools/openapi-generator-online to work as triggered builds for releases as well.

@jmini

This comment has been minimized.

Show comment
Hide comment
@jmini

jmini Aug 20, 2018

Member

Could the 2 files be under the CI/ folder?
I think @wing328 tries to put everything related to CI in this folder.

If I understood this correctly, if we use "Docker Hub for automated builds" we will no longer push images from "travis". Should we disable docker-push from travis within the same PR? Otherwise how will we be able to see if your setup of "Docker Hub for automated builds", if we have travis also pushing images?

For me the appropriate moment to do the switch would be right after the next release 3.2.2 (due date tomorrow) ...

Member

jmini commented Aug 20, 2018

Could the 2 files be under the CI/ folder?
I think @wing328 tries to put everything related to CI in this folder.

If I understood this correctly, if we use "Docker Hub for automated builds" we will no longer push images from "travis". Should we disable docker-push from travis within the same PR? Otherwise how will we be able to see if your setup of "Docker Hub for automated builds", if we have travis also pushing images?

For me the appropriate moment to do the switch would be right after the next release 3.2.2 (due date tomorrow) ...

@jimschubert

This comment has been minimized.

Show comment
Hide comment
@jimschubert

jimschubert Aug 20, 2018

Member

If we put it under CI, it complicates the context because Docker hub doesn't allow you to define the build context path. I'd rather keep these as hidden files in the root.

Once this is in master, we can merge to the versioned branches and evaluate. There's no need to remove the Travis step until the automated build stuff is working as expected.

We'll need to figure out how we'd want to deprecate the other two Docker repos, are if there's a way to trigger a build of those based on the automated build. The latter might be confusing.

Member

jimschubert commented Aug 20, 2018

If we put it under CI, it complicates the context because Docker hub doesn't allow you to define the build context path. I'd rather keep these as hidden files in the root.

Once this is in master, we can merge to the versioned branches and evaluate. There's no need to remove the Travis step until the automated build stuff is working as expected.

We'll need to figure out how we'd want to deprecate the other two Docker repos, are if there's a way to trigger a build of those based on the automated build. The latter might be confusing.

Merge branch 'master' into hub-dockerfile
* master:
  Fix problems in typescript jquery generator (#801)
  [PHP] Add gitignore to AbstractPhpCodegen (#765)
  Improve documentation for usage of a generator in an other jar (#817)
  Improve Symfony 4.1 compatibility (#830)
  📝 Updating 'help generate' switches in README
  [cli] Don't log to STDOUT if debug flags are set (#474)
  [gradle-plugin] README notes on multiple specs (#847)
  Added server variable support (#816)
  fix erlang optiona/required parameters (#829)
  [Java][Rest-assured] Fix generated javadoc and "swagger-annotations" improvement (#831)
@jimschubert

This comment has been minimized.

Show comment
Hide comment
@jimschubert

jimschubert Aug 21, 2018

Member

@jmini in case you're not familiar with the aspect of Docker… if you move the files under CI it means we have to change COPY . to COPY .., but you're not allowed to pull in files outside of the build context. I assume Docker does this for security reasons. And because Docker hub doesn't allow us to specify the build context location, you'd end up with this:

$ docker build -t cli-docker -f CI/cli.dockerfile .
Sending build context to Docker daemon  400.1MB
Step 1/15 : FROM jimschubert/8-jdk-alpine-mvn:1.0 as builder
 ---> 240eccea6c3b
Step 2/15 : RUN set -x &&     apk add --no-cache bash
 ---> Using cache
 ---> f01cc0142bc9
Step 3/15 : ENV GEN_DIR /opt/openapi-generator
 ---> Using cache
 ---> 864d258afe67
Step 4/15 : WORKDIR ${GEN_DIR}
 ---> Using cache
 ---> 073f17ab9a38
Step 5/15 : COPY ../ ${GEN_DIR}
COPY failed: Forbidden path outside the build context: ../ ()

I think having the files as hidden files in the root for the benefit of having an automated build is a fair trade-off.

Member

jimschubert commented Aug 21, 2018

@jmini in case you're not familiar with the aspect of Docker… if you move the files under CI it means we have to change COPY . to COPY .., but you're not allowed to pull in files outside of the build context. I assume Docker does this for security reasons. And because Docker hub doesn't allow us to specify the build context location, you'd end up with this:

$ docker build -t cli-docker -f CI/cli.dockerfile .
Sending build context to Docker daemon  400.1MB
Step 1/15 : FROM jimschubert/8-jdk-alpine-mvn:1.0 as builder
 ---> 240eccea6c3b
Step 2/15 : RUN set -x &&     apk add --no-cache bash
 ---> Using cache
 ---> f01cc0142bc9
Step 3/15 : ENV GEN_DIR /opt/openapi-generator
 ---> Using cache
 ---> 864d258afe67
Step 4/15 : WORKDIR ${GEN_DIR}
 ---> Using cache
 ---> 073f17ab9a38
Step 5/15 : COPY ../ ${GEN_DIR}
COPY failed: Forbidden path outside the build context: ../ ()

I think having the files as hidden files in the root for the benefit of having an automated build is a fair trade-off.

@wing328

This comment has been minimized.

Show comment
Hide comment
@wing328

wing328 Aug 21, 2018

Member

For this case, I think it's totally ok to leave the files in the root directory instead of putting them under the CI/ folder.

Member

wing328 commented Aug 21, 2018

For this case, I think it's totally ok to leave the files in the root directory instead of putting them under the CI/ folder.

@jmini

This comment has been minimized.

Show comment
Hide comment
@jmini

jmini Aug 21, 2018

Member

I was just asking a question, not requesting a change...

If you are sure that this change do not break the release we will perform tomorrow and that "docker push from travis" can still be used in parallel to this automated docker build (which seems to be the case), this PR is harmless.

Member

jmini commented Aug 21, 2018

I was just asking a question, not requesting a change...

If you are sure that this change do not break the release we will perform tomorrow and that "docker push from travis" can still be used in parallel to this automated docker build (which seems to be the case), this PR is harmless.

@jimschubert

This comment has been minimized.

Show comment
Hide comment
@jimschubert

jimschubert Aug 21, 2018

Member

@jmini yeah it doesn't affect the Dockerfile in either of the published images. I left those as-is because people use them for local development and that use case wouldn't work well with a jar-only image.

Member

jimschubert commented Aug 21, 2018

@jmini yeah it doesn't affect the Dockerfile in either of the published images. I left those as-is because people use them for local development and that use case wouldn't work well with a jar-only image.

@wing328 wing328 added this to the 3.2.2 milestone Aug 21, 2018

@wing328

This comment has been minimized.

Show comment
Hide comment
@wing328

wing328 Aug 21, 2018

Member

@jimschubert thanks for the enhancement. Let's move forward with the change.

Member

wing328 commented Aug 21, 2018

@jimschubert thanks for the enhancement. Let's move forward with the change.

@wing328 wing328 merged commit cbf61d5 into master Aug 21, 2018

6 checks passed

Shippable Run 2333 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment