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

Add .dockerignore #328

Merged
merged 2 commits into from
Nov 10, 2020
Merged

Add .dockerignore #328

merged 2 commits into from
Nov 10, 2020

Conversation

tczekajlo
Copy link
Contributor

Proposed changes:

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@tczekajlo tczekajlo self-assigned this Nov 4, 2020
@tczekajlo
Copy link
Contributor Author

@m-vdb @wochinge Regarding #322 I assume we don't need to and want to put the docs directory in the rasa-sdk image?

@wochinge
Copy link
Contributor

wochinge commented Nov 4, 2020

haha, please not! 😂

@tczekajlo tczekajlo requested review from wochinge and m-vdb November 4, 2020 15:13
@m-vdb
Copy link
Collaborator

m-vdb commented Nov 4, 2020

it also seems that we only need rasa_sdk and entrypoint.sh, am I right? should ignore everything else?

@wochinge
Copy link
Contributor

wochinge commented Nov 5, 2020

@m-vdb We need at least the poetry files as well.

I was wondering: Do we need this line in the Dockerfile? In my opinion we

  • can only copy the rasa_sdk directory to the next stage
  • or (I think even better) is to don't install rasa_sdk in editable mode but actually build the wheel and copy that one over as we do it in the rasa image (see here. See also this poetry issue

I think this approach would be more robust than just doing the .dockerignore (might also be done in combination)

@tczekajlo
Copy link
Contributor Author

@m-vdb We need at least the poetry files as well.

I was wondering: Do we need this line in the Dockerfile? In my opinion we

  • can only copy the rasa_sdk directory to the next stage
  • or (I think even better) is to don't install rasa_sdk in editable mode but actually build the wheel and copy that one over as we do it in the rasa image (see here. See also this poetry issue

I think this approach would be more robust than just doing the .dockerignore (might also be done in combination)

My plan was to take care of the rasa-sdk image after https://github.com/RasaHQ/rasa-x/pull/3886 is done.

For now, I propose to add the .dockerignore, and in not far future take care of Docker build as well, wdyt?

@m-vdb
Copy link
Collaborator

m-vdb commented Nov 9, 2020

works for me 👍

Copy link
Collaborator

@m-vdb m-vdb left a comment

Choose a reason for hiding this comment

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

📟

@wochinge
Copy link
Contributor

wochinge commented Nov 9, 2020

works for me as well. Can we also exclude tests then? .github should be excluded by default, right?

@tczekajlo
Copy link
Contributor Author

tczekajlo commented Nov 10, 2020

works for me as well. Can we also exclude tests then? .github should be excluded by default, right?

It copies everything 😄 I'll add a few more files
Screenshot 2020-11-10 at 13 28 05

@wochinge
Copy link
Contributor

Thanks for adding the missing entries 🚀 Having our .git history in the docker images seems unnecessary 😆

@tczekajlo
Copy link
Contributor Author

Thanks for adding the missing entries 🚀 Having our .git history in the docker images seems unnecessary 😆

Distributed backup 😄

@tczekajlo tczekajlo merged commit b3b48c2 into master Nov 10, 2020
@tczekajlo tczekajlo deleted the fix_sec branch November 10, 2020 13:32
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.

Security Alert
3 participants