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

Poetry for rasa-sdk 🎉 #145

Merged
merged 18 commits into from
Feb 13, 2020
Merged

Poetry for rasa-sdk 🎉 #145

merged 18 commits into from
Feb 13, 2020

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Feb 4, 2020

Proposed changes:

  • Add poetry to rasa-sdk
  • Update Dockerfile (=> reduce the size of Docker image from 400 to about 200 Mb)
  • Update dependencies

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
  • reformat files using black (please check Readme for instructions)

IMPORTANT: Things to do before merging this PR:

@alwx alwx changed the title Poetry for rasa-sdk (WIP) Poetry for rasa-sdk Feb 4, 2020
@alwx alwx requested review from tmbo, wochinge and ricwo February 4, 2020 16:41
@alwx alwx changed the title Poetry for rasa-sdk Poetry for rasa-sdk 🎉 Feb 4, 2020
tmbo
tmbo previously requested changes Feb 5, 2020
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

great work - looks a lot easier to maintain 🎉

I've added a couple comments regarding metadata that is missing and some open questions.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@tmbo
Copy link
Member

tmbo commented Feb 5, 2020

one more open question: does publishing the package using travis deploy still work?

.travis.yml Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Great work @alwx 💯

.travis.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@alwx
Copy link
Contributor Author

alwx commented Feb 6, 2020

Now it also includes changes from #146

Additional changes:

  • docker image is now based on python3.6-slim, not alpine
  • scripts/release.py automatically changes version in pyproject.toml as well
  • all the dependencies have been updated
  • metadata in pyproject.toml
  • deploy build step in .travis.yml updated

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

🎉 Stoked for poetry :-)

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
rasa_sdk/version.py Outdated Show resolved Hide resolved
scripts/release.py Show resolved Hide resolved
scripts/release.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@alwx alwx force-pushed the poetry branch 4 times, most recently from 6cfb6fc to d4b1a41 Compare February 7, 2020 17:00
.travis.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@alwx
Copy link
Contributor Author

alwx commented Feb 10, 2020

Things to do before merging this PR:

@tmbo tmbo dismissed their stale review February 10, 2020 14:02

outdated

@tmbo
Copy link
Member

tmbo commented Feb 10, 2020

@alwx is the used password env already the correct one?

@alwx alwx requested a review from wochinge February 10, 2020 14:03
@alwx
Copy link
Contributor Author

alwx commented Feb 10, 2020

@tmbo no, it's not — it should be updated first.

.travis.yml Outdated Show resolved Hide resolved
@tmbo
Copy link
Member

tmbo commented Feb 10, 2020

I've encrypted the secret and suggested the change @alwx

@alwx alwx removed the request for review from ricwo February 11, 2020 08:23
.travis.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
changelog/145.improvement.rst Outdated Show resolved Hide resolved
@alwx alwx requested review from tmbo and wochinge February 11, 2020 10:22
@wochinge
Copy link
Contributor

@alwx Why did we remove coveralls?

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Looks great 💯

@alwx alwx mentioned this pull request Feb 12, 2020
2 tasks
@alwx alwx merged commit 1ef753e into master Feb 13, 2020
@tmbo tmbo deleted the poetry branch February 13, 2020 15:39
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.

3 participants