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

Add support for GitHub actions #837

Closed
wants to merge 4 commits into from
Closed

Conversation

jocelynj
Copy link
Contributor

This would be very useful to automatically check commits and pull-requests with current unit tests.

@tkeffer
Copy link
Contributor

tkeffer commented Jan 27, 2023

This is great!

Are you willing to maintain it? I don't think it would be a big job for someone who understands CI. Mostly responding to issues and questions, with an occasional feature request.

@jocelynj
Copy link
Contributor Author

Yes, I should be able to maintain it, as I use github CI on other projects.

There are a lot of things that could be added to CI - for example, check all supported python versions, or do some code linting.

@tkeffer
Copy link
Contributor

tkeffer commented Jan 29, 2023

Merged into branch development. Commit 147500c

@tkeffer tkeffer closed this Jan 29, 2023
@jocelynj jocelynj deleted the github-actions branch January 29, 2023 19:44
@tkeffer
Copy link
Contributor

tkeffer commented Feb 6, 2023

First issue. A CI test is failing because a Pillow function is missing. I suspect the problem is that the CI process is installing an old version of Pillow, which does not have the function.

We require something greater than V9.2 (current version is 9.4). Is there any way to see what version is being installed by apt-get install python3-pillow?

@jocelynj
Copy link
Contributor Author

jocelynj commented Feb 6, 2023

Looks like python3-pillow is a virtual package pointing to python3-pil: https://packages.debian.org/stable/python3-pillow

Log on CI says that we are installing this version:
Unpacking python3-pil:amd64 (9.0.1-1ubuntu0.1) ...

So, a bit too old.

What I can do is add a step to run pip3 install -r requirements.txt, but I see that requirements.txt was removed ton the V5 branch, so I'm not sure what to do. Maybe I could add a pip3 install Pillow>=9.2 in the CI setup - what do you think?

@tkeffer
Copy link
Contributor

tkeffer commented Feb 6, 2023

The problem is only in the V5 branch, which includes workarounds for some deprecations in Pillow.

So, how about leave master and development alone, but use pip to do the installs for the V5 tests? Best of all would be to use poetry to build a wheel, which would have the correct dependencies, then test from that, but that may be a bit too much.

@vinceskahan
Copy link
Contributor

I got an email the other day saying that my fork of weewx was (much to my surprise) running CI tasks when I push to my fork. I suspect the other 280 fork owners would be similarly surprised and possibly confused by such unexpected email from github. While we can individually turn this off by digging through the docs and setting parameters for each repo we fork, shouldn't the default be 'off' for forks ?

I found one reference that seems to say a quick addition to your CI setup might do the trick if you look at the suggested solution here - https://github.com/orgs/community/discussions/26704#discussioncomment-3252979

It seems that a one-liner on line 8 of ci.yaml might restrict running the CI suite to only the real repo (not the forks):
if: github.repository == ‘weewx/weewx’

I don't speak github CI well enough to know if this would restrict any other use cases folks have. It's of course possible that some folks with forks might 'want' the CI stuff to run for their forks too, so I guess give it some thought please.

@jocelynj
Copy link
Contributor Author

jocelynj commented Feb 8, 2023

That's a good point, but I don't have a good answer: it would be best if CI was disabled by default on github forks. Note two things:

  • forks will get this CI only if they fetch the latest master from weewx, as they need to get the .github/workflows/ci.yml file
  • forks can still be interested in running CI. For example, I would like to test my changes on my own repository before making a PR.

@vinceskahan
Copy link
Contributor

Looks to me like the CI stuff is on for the currently active branches (master, development, V5).

Problem is that posting about this one to the -development list might confuse folks more than just waiting for the next person to encounter it and ask.

Would a small 'Forking the WeeWX GitHub repository' section ala my comment above in the Developer's Notes document perhaps be worth thinking about ? It did take a little searching to find the easy procedure to let me disable that stuff for my fork.

I'd be willing to add a section to the Developer's Notes if you+Tom want.

@jocelynj
Copy link
Contributor Author

@vinceskahan : this seems a good idea, but I'm not a weewx mainteneur :)

@tkeffer : do you agree with the documentation update above?

@tkeffer
Copy link
Contributor

tkeffer commented Feb 17, 2023

Seems like a good idea.

@vinceskahan
Copy link
Contributor

ok - let me add a brief paragraph to the Developer's Notes in a separate PR. Thanks.

@vinceskahan
Copy link
Contributor

@jocelynhj @tkeffer done - #846

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.

3 participants