-
Notifications
You must be signed in to change notification settings - Fork 1
Add reusable workflow for building and publishing python packages with poetry #3
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
Conversation
…akdata/ci-templates into feature/add-python-release-with-poetry
|
I think we should separate reusable workflows from composite actions inside the repository. We could use a directory structure like this for example: I would also rename Additionally we should think about splitting the workflow up into smaller actions, so that individual parts of it can be reused as well |
|
@yannick-roeder & @disrupted I applied the changes that we discussed yesterday. The PR is so far ready to review. Before you start the review some notes on the changes:
|
Co-authored-by: Salomon Popp <mail@salomonpopp.me>
Co-authored-by: Salomon Popp <mail@salomonpopp.me>
Co-authored-by: Salomon Popp <mail@salomonpopp.me>
Co-authored-by: Salomon Popp <mail@salomonpopp.me>
disrupted
left a comment
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 looks really great! I just commented some minor nitpicks.
|
I noticed that the changelog creation is not part of the workflow. Did you leave it out intentionally? Of course this could also be added in a followup, I think that this could be really useful to other projects too. |
|
Yes, changelog generation would be nice for other projects as well. I think we can resolve that in a separate issue |
…akdata/ci-templates into feature/add-python-release-with-poetry
|
@disrupted Yeah, I didn't add it here yet since it is complicated action and needs some configuration. I would also instead do it in a separate PR. |
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.
LGTM
The changes and reviews are already commited
fixes #5