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

try docker release workflow #67

Closed
wants to merge 4 commits into from
Closed

try docker release workflow #67

wants to merge 4 commits into from

Conversation

8bitsam
Copy link
Contributor

@8bitsam 8bitsam commented Aug 7, 2024

Add a release.yml workflow file that should use docker to release on pypi. I also fixed a problem where the stage on pre-commit-config.yaml wasn't right.

@8bitsam
Copy link
Contributor Author

8bitsam commented Aug 7, 2024

This should build using github's docker setup, might be easier than building our own for now.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks Sam, this is great.

please see comment on the pre-commit.

This looks good. @Sparks29032 please can you review?

I think we will need an action for building the changelog from the news. Is there anything else that is being handled by the bash scripts that should go here?

I think we discussed before whether we should build and deploy from actions and decided against it for some reason but I don't remember why. Andrew, do you remember? If this works for pdffit2, should we do all the releasing from here?

This seems to be on push/ Does it mean that a new release is done every time we merge into main? How do we specify the new release number?

@@ -42,4 +42,4 @@ repos:
- id: no-commit-to-branch
name: Prevent Commit to Main Branch
args: ["--branch", "main"]
stages: [pre-commit]
stages: [commit]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure this should be pre-commit. Why do you think it should be commit?

Copy link
Contributor

@Sparks29032 Sparks29032 Aug 8, 2024

Choose a reason for hiding this comment

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

From pre-commit.com: "new in 3.2.0: The values of stages match the hook names. Previously, commit, push, and merge-commit matched pre-commit, pre-push, and pre-merge-commit respectively."

Yea, we should be using pre-commit as the name of the hook as we have the pre-commit.ci auto-updating the pre-commit version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to edit it since for some reason pre-commit run was failing due to the name being pre-commit instead of commit. Not sure if this problem is present for anyone else?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a workaround not a fix presumably, so let's try and figure out why this is happening and do a proper fix. Always aim to make code better, not make code run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it. Though it might be due to local configs, I still get this error:

An error has occurred: InvalidConfigError: 
==> File .pre-commit-config.yaml
==> At Config()
==> At key: repos
==> At Repository(repo='https://github.com/pre-commit/pre-commit-hooks')
==> At key: hooks
==> At Hook(id='no-commit-to-branch')
==> At key: stages
==> At index 0
=====> Expected one of commit, commit-msg, manual, merge-commit, post-checkout, post-commit, post-merge, post-rewrite, prepare-commit-msg, push but got: 'pre-commit'

Copy link
Contributor

Choose a reason for hiding this comment

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

make sure your local pre-commit is up to date?

@Sparks29032
Copy link
Contributor

Thanks Sam, this is great.

please see comment on the pre-commit.

This looks good. @Sparks29032 please can you review?

I think we will need an action for building the changelog from the news. Is there anything else that is being handled by the bash scripts that should go here?

I think we discussed before whether we should build and deploy from actions and decided against it for some reason but I don't remember why. Andrew, do you remember? If this works for pdffit2, should we do all the releasing from here?

This seems to be on push/ Does it mean that a new release is done every time we merge into main? How do we specify the new release number?

The bash scripts will still need to be run to create the tag and upload to a GitHub release. This seems to only handle PyPi releases correct? We will need to put the secrets (PyPi user and pass) into each GitHub repository we want to release with this approach, which seems like a pain since updating the password on PyPi would require us to change the password secret on every GitHub repository we will release using this approach. If we are able to, let's try to either add a workflow like this to the release scripts or make the docker ourselves.

Still setting up my Ubuntu, but this seems conditioned on setup.py. Can we replace python setup.py sdist bdist_wheel with some variation of python -m build?

Also, has this been tested? It seems we will need to make a dummy repository and branch that sends items to PyPi in order to test this workflow.

@8bitsam
Copy link
Contributor Author

8bitsam commented Aug 8, 2024

I switched it to use build, also I could make a repo to test it on. We could change the on: push part to on: workflow_dispatch so it could be run with CLI? I think that might be a better option since it's more controlled.

Also yes, this only handles PyPI releases. I figured it's easier than making a whole docker setup locally since the github matrix os seems like it should be able to handle that effectively.

@sbillinge
Copy link
Contributor

I switched it to use build, also I could make a repo to test it on. We could change the on: push part to on: workflow_dispatch so it could be run with CLI? I think that might be a better option since it's more controlled.

Also yes, this only handles PyPI releases. I figured it's easier than making a whole docker setup locally since the github matrix os seems like it should be able to handle that effectively.

Don't we want the GitHub release payload to also have all the wheels though? Does this actually solve our problem?

I am ok with a github workflow approach in general, but only if it is the best solution.

I don't think we want to keep pypi secrets in the repo. Isn't this a security issue?

I didn't spend a lot of time with it, but it seems that actions/upload-artifact is based on actions/artifact which maybe builds the artifacts but doesn't upload them? This would seem to be a better match to what we want? This one could even be run on push maybe if it just builds the wheels but doesn't do the release on push. Then if our current release workflow knows where to find the pre-built wheels and doesn't try and build them, but puts them into a payload for GH and then for PyPi? Something like that could be attractive possibly?

@8bitsam
Copy link
Contributor Author

8bitsam commented Aug 8, 2024

Don't we want the GitHub release payload to also have all the wheels though? Does this actually solve our problem?

I am ok with a github workflow approach in general, but only if it is the best solution.

After doing some research, it seems like this is probably the most efficient way to do the releases. I changed the workflow so it can only be manually activated, that way we can choose exactly when to release it. I also added a second job that publishes the release to PyPI.

I don't think we want to keep pypi secrets in the repo. Isn't this a security issue?

The secrets thing shouldn't be a security issue if the proper workflow for setting it up is followed here.

I think we can also have this workflow create a new tag for us with the github deploy action, so we don't need to use the bash script.

@8bitsam
Copy link
Contributor Author

8bitsam commented Aug 8, 2024

I didn't spend a lot of time with it, but it seems that actions/upload-artifact is based on actions/artifact which maybe builds the artifacts but doesn't upload them? This would seem to be a better match to what we want? This one could even be run on push maybe if it just builds the wheels but doesn't do the release on push. Then if our current release workflow knows where to find the pre-built wheels and doesn't try and build them, but puts them into a payload for GH and then for PyPi? Something like that could be attractive possibly?

I think that by using upload-artifacts, we can store the wheel files, but just using artifacts won't allow us to do that. This way we can also handle the actual publishing to PyPI on the same workflow.

Another thing we can do is make a similar workflow to release on conda-forge, that way all the releasing could be handled from two github workflows.

@sbillinge
Copy link
Contributor

@Sparks29032 please chime in. I know we discussed this before and chose not to do it this way for some reasons.

Also, I don't see the news workflow in the current action? Is this possible if we switch to GH actions? Let's keep the convo going. We worked very hard to get the shell scripts working, and they are working, so there has to be a significant benefit to do the work to switch it. I expect this work would fall to Sam.....

@Sparks29032
Copy link
Contributor

Sparks29032 commented Aug 8, 2024

I am a bit biased toward scripts as we can be more expressive in Python. With these workflows we will have to update them in every package if we want changes in behavior while the release repository makes it centralized.

We will end up having to use scripts anyways for news and automatic version control and building the api documentation.

The conda-forge workflow is a bit different than pypi as the only upload is the meta.yaml, but there may be workflows for this also.

Let's also try testing this upload pathway works with a dummy repository. I have one repo set up already, but my computer is still not fully set up yet.

@sbillinge
Copy link
Contributor

Let's not pursue this further at this point for doing releases. We worked hard to get things automated (that was the goal of this entire project), so I am reluctant to change to a different way of doing it unless it is absolutely clear it is superior. If , after a couple of years, we find issues with the bash scripts we can revisit this then.

If there are useful matrix workflows for building wheels on different platforms that we can then reuse in the scripts, this would seem to be of some value. We need to do the snake-nest for getx3 releases though in any case, so I guess if we figure this out here, we can reuse a lot of that stuff for the getx3 releases.

@bobleesj
Copy link
Contributor

@8bitsam @sbillinge could we close this PR? A good discussion though but now we do have wheel files built via GitHub Actions.

@sbillinge
Copy link
Contributor

Closing as no longer needed. Replaced by GH workflows

@sbillinge sbillinge closed this Nov 22, 2024
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.

4 participants