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

GitHub actions: Create release.yml #68

Merged
merged 9 commits into from
Apr 27, 2021
Merged

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented Mar 31, 2021

Description of the change

Setting up the github actions to automate releasing when a new release tag is pushed. Based on hyperspy's release.yml.

Progress of the PR

  • Adapted file from hyperspy_gui_ipywidgets,
  • set pypi secrets,
  • test run in fork,
  • add MANIFEST.in
  • add changelog,
  • Add releasing_guide.md to have the recipe for a release summarized,
  • Add keywords to release_info.py,
  • ready for review.

@codecov-io
Copy link

codecov-io commented Mar 31, 2021

Codecov Report

Merging #68 (34a0cde) into master (72c9a09) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   95.89%   95.89%           
=======================================
  Files          12       12           
  Lines         536      536           
=======================================
  Hits          514      514           
  Misses         22       22           
Impacted Files Coverage Δ
lumispy/release_info.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72c9a09...34a0cde. Read the comment docs.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@jlaehne jlaehne changed the title Create release.yml GitHub actions: Create release.yml Mar 31, 2021
@jlaehne jlaehne mentioned this pull request Mar 31, 2021
Copy link
Contributor

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

I have left a few comments.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@ericpre
Copy link
Contributor

ericpre commented Apr 1, 2021

Can you try to push a tag in your fork to check that it triggers the workflow and that everything goes well? Normally, everything should pass and the last step with pushing to pypi should be skipped.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 1, 2021

I did. It fails, but at the same point as for hyperspy_gui_ipywidgets: https://github.com/jlaehne/lumispy/runs/2249602259?check_suite_focus=true

So is that sufficient?

For the moment I dropped automatic writing of the version, because we have the version in release_info.py and as far as I see the solution from hyperspy_gui_ipywidgets overwrites the full file.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 2, 2021

Maybe we could stick with manually setting the version in release_info.py, but do a check in the github action that the version in the file (can be read as $(python setup.py --version)) is the same as in the tag?

@ericpre
Copy link
Contributor

ericpre commented Apr 2, 2021

Yes, what I have experimenting with updating the version is not great and it can be done manually as you suggest. There is an setuptools-scm which seems to be worth considering, but even if these things have changed fluid in the last few years, maybe it has now converged to something useful?!

In case of https://github.com/jlaehne/lumispy/runs/2249602259?check_suite_focus=true, this is failing at the very beginning when creating the github release and according to error message, the release already exists:

Error: Validation Failed: {"resource":"Release","code":"already_exists","field":"tag_name"}

Is it possible that it already exists from a previous run with the same tag?

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 2, 2021

I tried with new tag, but the same: https://github.com/jlaehne/lumispy/actions/runs/711368311

@ericpre
Copy link
Contributor

ericpre commented Apr 2, 2021

I am not sure which branch I should be looking at, the one in lumispy/lumispy or in jlaehne/lumispy? This PR is from a branch from lumispy/lumispy.

@ericpre
Copy link
Contributor

ericpre commented Apr 2, 2021

I did the following:

  • add a git tag (v0.1.1.test)
  • push the tag to ericpre/lumispy

and it works fine: https://github.com/ericpre/lumispy/actions/runs/711407096

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 2, 2021

So far I had used Draft a New Release to create the tag. Doing it on the command line worked now:
https://github.com/jlaehne/lumispy/actions/runs/711462046

This should be the issue: a git tag and a github release are two different things: https://docs.github.com/en/github/administering-a-repository/about-releases. I think that the most interoperable way is to create "git tag" manually instead of creating the github release manually.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 2, 2021

We still need to decide how to track the CHANGES. The solution in HyperSpy should be changed again, as far as I understood.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 2, 2021

@ericpre Do you have an idea how to simply verify that the version in release_info.py and the current tag are equal? Should be enough for versioning, but would be a sanity check.

@ericpre
Copy link
Contributor

ericpre commented Apr 2, 2021

We still need to decide how to track the CHANGES. The solution in HyperSpy should be changed again, as far as I understood.

In hyperspy, this is annoying because there is more traffic and a number of PRs take more time to get merged, which is the source of conflict of the change log...
To have it automated, towncrier seems a good option, it used by many repository on github and for example pipenv uses it.

@ericpre Do you have an idea how to simply verify that the version in release_info.py and the current tag are equal? Should be enough for versioning, but would be a sanity check.

As mentioned above, I would check if setuptools-scm can be useful for that.

@jordiferrero
Copy link
Contributor

Let me know if you need any help with this.
I am a bit unsure how to help if I am honest. But happy to learn!

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 13, 2021

In principle the action is running. I was trying to add a consistency check between the version in the tag and the version in release_info.py as a condition for the rest of the action to run, but I have not really progressed on that so far. I have not found time to dig deeper into setuptools-scm as alternative, but I thought we are not that far with the project that we really need those extras yet. We could live with ensuring the right versioning manually for the moment, I guess.

Concerning the changelog, there is the solution proposed by @ericpre using towncrier. But also there are a large number of Github actions that would compile a changelog from the PRs that have been merged, which would probably be enough automation for us. But then, which one to choose. In HyperSpy, the changelog file is updated manually, but that currently leads to a lot of merge conflicts that have to be resolved manually. I guess for the moment we could even live with that, but then it does not hurt to do things right.

Just had a look at pyxem and they simply do all that manually. So how about keeping these as nice to have features for the future if one of us finds the time to dig deeper? Then we can focus on adding other functionalities for the moment.

@jordiferrero
Copy link
Contributor

I'm happy with not worrying too much on this.
Since it's only 3 of us working on this project we should be fine for now.
I've added it in the project "Nice features for the future".
If you all agree, we can merge and close this for now.

@ericpre
Copy link
Contributor

ericpre commented Apr 14, 2021

In principle the action is running. I was trying to add a consistency check between the version in the tag and the version in release_info.py as a condition for the rest of the action to run, but I have not really progressed on that so far.

Yes, this is good idea to check that there is no mistake in the version... I would write a python script comparing the tag, which is available as environment variable and compare it with the lumispy.__version__. Maybe it could fit in one python line, which could be added to the workflow without adding a file to lumispy.

Regarding the changelog, it can indeed be done manually!

Copy link
Contributor

@ericpre ericpre left a comment

Choose a reason for hiding this comment

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

This looks good to me. I ran a workflow in https://github.com/ericpre/lumispy/actions/runs/711407096 and it went through until the end. The only thing left to check is if the secrets are set correctly for pypi and there is no other way than actually doing it! If it fails, this is not a big deal, just need to delete the tag and the github release and make sure if there are correct and try it again.

@jordiferrero
Copy link
Contributor

What do you mean by "another way to store the secrets"? Isn't the Github way the correct way?

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 14, 2021

Yes, this is good idea to check that there is no mistake in the version... I would write a python script comparing the tag, which is available as environment variable and compare it with the lumispy.__version__. Maybe it could fit in one python line, which could be added to the workflow without adding a file to lumispy.

In principle it should work like that and I will give it a try as soon as I find a quiet moment, maybe later this week. There is just a bit of other stuff that has piled up over the easter holidays.

Regarding the changelog, it can indeed be done manually!
Yes, I will simply create the file in this PR.

Once everything is in, I will do the next minor realease to test the workflow.

@ericpre
Copy link
Contributor

ericpre commented Apr 14, 2021

What do you mean by "another way to store the secrets"? Isn't the Github way the correct way?

Yes, setting the github secrets is good, but since this is not possible to check after there have been set and it is always possible to have a typo or a mistake in the the copy and paste, etc. For example, it happen to me recently!

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* test version

* add changelog

* add changelog
@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2021

Codecov Report

Merging #68 (8cc3a50) into master (dea2c62) will decrease coverage by 0.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
- Coverage   95.89%   95.52%   -0.38%     
==========================================
  Files          12       12              
  Lines         536      536              
==========================================
- Hits          514      512       -2     
- Misses         22       24       +2     
Impacted Files Coverage Δ
lumispy/release_info.py 100.00% <100.00%> (ø)
lumispy/signals/cl_spectrum.py 94.28% <0.00%> (-2.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dea2c62...8cc3a50. Read the comment docs.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 25, 2021

Release workflow runs through when version is correct: https://github.com/jlaehne/lumispy/actions/runs/783664680
but fails if there is a mismatch between release_info.py and the tag: https://github.com/jlaehne/lumispy/actions/runs/783667655

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 25, 2021

@jordiferrero should be ready now, though I do not know why the tests of the grating_shift suddenly fail, because they were not touched - could it be a result of the new patch release of HyperSpy @ericpre?

@ericpre
Copy link
Contributor

ericpre commented Apr 26, 2021

Indeed, the failure are related to changes in the map method:

ValueError: The size of the navigation_shape for the kwarg shift (<(100,)> must be consistent with the size of the mapped signal <(10, 10)>

This point out to an argument which doesn't have the right shape. It is fairly likely that ambiguous usage of map such as this one are not allowed anymore.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 26, 2021

OK, I created a separate issue #75 for the failing tests.

This one should be ready then.

@jlaehne
Copy link
Contributor Author

jlaehne commented Apr 26, 2021

By the way, I just registered lumispy.org, which forwards to this repository for the moment, but can be later used as link for the documentation.

@jlaehne jlaehne merged commit 8e041ab into master Apr 27, 2021
@jlaehne jlaehne added this to the v0.1.1 milestone Apr 27, 2021
@jlaehne jlaehne deleted the release-github-actions branch August 25, 2021 12:27
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.

5 participants