DeepRank / deeprank2

An open-source deep learning framework for data mining of protein-protein interfaces or single-residue variants.
https://deeprank2.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
32 stars 12 forks source link

ci: improve release workflow #621

Open DaniBodor opened 1 month ago

DaniBodor commented 1 month ago

An automated workflow can be triggered from the Actions tab (once this is merged to main) that creates a draft GitHub release. The draft release (notes) can be edited if needed, and then manually converted to an actual Release here, which will trigger the PyPi release (note that a draft release does not trigger PyPi release).

The workflow has the following logic:

This workflow was adapted from here.

fix #170

DaniBodor commented 1 month ago

Very nice improvements to the workflow :D

I added some comments, and requested changes since I'd like to be aware of them (the flow is a bit more complex now, I am like a new developer testing it). Also I wonder, has this workflow being tested? Maybe we can try to do a draft release (that then we cancel) before merging it into dev.

I've made some improvements, mainly to the documentation.

I've tested this in the eitprocessing repo, but not yet here. There are a few difficulties in testing this:

EDIT: newest tests on eitprocessing aren't working after all 💩 . I will figure it out there and then make changes here. Until then, this PR can stay open.

gcroci2 commented 1 month ago

EDIT: newest tests on eitprocessing aren't working after all 💩 . I will figure it out there and then make changes here. Until then, this PR can stay open.

Okay, I've added a couple of comments still to be addressed. Just ask for my review again when you'll discover what is the fix needed :)

DaniBodor commented 1 month ago

EDIT: newest tests on eitprocessing aren't working after all 💩 . I will figure it out there and then make changes here. Until then, this PR can stay open.

Okay, I've added a couple of comments still to be addressed. Just ask for my review again when you'll discover what is the fix needed :)

Will do. I think the best way to test this is to create a fork of the repo, test everything there, and then if it works implement it here as well.

gcroci2 commented 1 month ago

Is this ready to be reviewed again? @DaniBodor If not, maybe either remove me from the reviewers or make this PR a draft (otherwise I keep receiving notifications ;))

github-actions[bot] commented 1 month ago

This PR is stale because it has been open for 14 days with no activity.

DaniBodor commented 1 month ago

This action can now be tested on this test fork: https://github.com/EIT-ALIVE/deeprank2_test_release/. I tested it already and it worked. See if you can follow from the dev readme how to do it and if it's explained clearly enough.

Note that in the forked repo, I bugged the release-on-PyPi action, so that we can still check that it gets triggered, but then fails (I don't think it would have been allowed to release a different repo, but didnt want to find out that somehow it does).

Also, I believe all the issues you @gcroci2 flagged have been addressed, or did I miss anything?

github-actions[bot] commented 2 weeks ago

This PR is stale because it has been open for 14 days with no activity.

DaniBodor commented 11 hours ago

The issue is due to the toml file, where the current version of the package seen from bumpversion does not get updated, and thus the error.

Other remaining things before merging:

  • Non-expiring token

  • Suggestions

It turns out that the error in the versioning is due to another bump version that occurred in between me creating this PR and now, which still used the old system and didn't update the version listed in bump-my-version-settings. Anyway, I've now made the line to be updated in each file explicit (avoids potential isssues e.g. if a dependency by coincidence lists identical version number) and moved the bumpversion settings into a separate toml file to keep pyproject.toml tidier. (NB: I think it would be a good idea to move the ruff settings into a separate toml as well, but that can be done in a separate PR. I create #641 for this.)

See my comments 1/2 on non-expiring tokens

I've integrated all suggestions to the wording of the dev readme, and ensured that the workflow is aborted if dev (or main) branch was selected. Also, I've turned on fail-fast to avoid a situation where bump-my-version (or any other step) fails, but the action is listed as successful (which I think happened in your test runs).

DaniBodor commented 8 hours ago

I request changes to remember that we need to look at the security thing again. I added a couple of minor suggestions for the instructions. Also let me know if I have to test everything on the fork again (if you say is all good I'll avoid that)

OK, let me know what you decide. If you want to test again (only minor changes occurred to workflow itself), the please use this new fork instead of the old one. It was quicker to create a new fork rather than update the other one.

gcroci2 commented 5 hours ago

I request changes to remember that we need to look at the security thing again. I added a couple of minor suggestions for the instructions. Also let me know if I have to test everything on the fork again (if you say is all good I'll avoid that)

OK, let me know what you decide. If you want to test again (only minor changes occurred to workflow itself), the please use this new fork instead of the old one. It was quicker to create a new fork rather than update the other one.

  • [ ] Once we are done testing, we should delete both test forks.

I tried doing the test, but it gives me: "Releasing from main or dev branch is not permitted, please select a valid release branch." even if I am using a branch called gcroci2-release-test. See here the PR and here the failed action. @DaniBodor

DaniBodor commented 4 hours ago

I request changes to remember that we need to look at the security thing again. I added a couple of minor suggestions for the instructions. Also let me know if I have to test everything on the fork again (if you say is all good I'll avoid that)

OK, let me know what you decide. If you want to test again (only minor changes occurred to workflow itself), the please use this new fork instead of the old one. It was quicker to create a new fork rather than update the other one.

  • [ ] Once we are done testing, we should delete both test forks.

I tried doing the test, but it gives me: "Releasing from main or dev branch is not permitted, please select a valid release branch." even if I am using a branch called gcroci2-release-test. See here the PR and here the failed action. @DaniBodor

it's fixed