Bump github python versions #14
Conversation
Explicitly give codeql-analysis action the security-events: write permission so it still works even when the default GitHub Actions token is set to read-only.
Add privileges to codeql-analysis.yml
* Bump versions * Bump back Pillow due to test failure * Bump setuptools
We missed Flower in our version upgrades earlier somehow, proposing the upgrade here.
Add Spanish Articles
Update to upstream
Bump Flower Version
Bump python image
.github/workflows/pylint.yml
Outdated
| pylint bookwyrm/ | ||
|
|
||
| - uses: actions/checkout@v3 | ||
| - name: Set up Python 3.9 |
There was a problem hiding this comment.
We should change this line to match the version, since it now sets up Python 3.11.
There was a problem hiding this comment.
The problem I ran into last time is not matching what upstream does. I don't know that I feel confident taking on the whole of upgrading a python version for this repo? Thoughts so, so welcome.
There was a problem hiding this comment.
So, the line after this actually sets the version, and it's already on 3.11 here, so this comment was more about updating the name to match what it does.
Upstream has changed everything significant that needs to change to support this upgrade already, though (that was part of the huge batch of changes we took recently). I think this would be fine for us at this point, and if we get have it working well in our prod that's a great argument to commit a similar upgrade to upstream.
Dockerfile
Outdated
| @@ -1,4 +1,4 @@ | |||
| FROM python:3.9 | |||
| FROM python:3.12.0b4-slim | |||
There was a problem hiding this comment.
We're currently on 3.11 in this file, but I think we should stay there?
There was a problem hiding this comment.
Actually, should this be using 3.11-bookworm? We're mostly using it but not always.
There was a problem hiding this comment.
Consistency is good; where are we using bookworm?
There was a problem hiding this comment.
Ah, saw below. Yeah, maybe try it and see what happens?
There was a problem hiding this comment.
Yeah, I don't know if there's a reason it's bookworm in one place but not the other, but since IIRC we are actually running this on the platform bookworm is meant for, I expect it should be a useful switch for us? I'll doublecheck the convo about where upstream started using bookworm for if there was a technical reason for the discrepancy versus just "one dockerfile is more hidden than the other".
There was a problem hiding this comment.
I checked upstream and they're using bookworm for dev-tools but not the main dockerfile. Let's stay aligned with them, and do regular 3.11 here?
There was a problem hiding this comment.
I was not able to find any discussion of why, because "bookworm" is a very common typo of "bookwyrm" and I'm not sure which python version Bookwyrm was on when Bookworm was introduced.
There was a problem hiding this comment.
Aha! I found it after all. bookwyrm-social#3190 was fixed in bookwyrm-social#3216 by using Bookworm for dev-tools. Let's stay consistent with upstream, since that shouldn't be necessary here.
dev-tools/Dockerfile
Outdated
| @@ -1,4 +1,4 @@ | |||
| FROM python:3.9 | |||
| FROM python:3.12.0b4-slim | |||
There was a problem hiding this comment.
We're currently on python:3.11-bookworm here, and I think we probably want to stay there.
I seem to have accidentally removed three dependencies while resolving merge conflicts. Putting them back now.
|
I believe Pylint is failing because |
After discussion of the deprecation of the imghdr module, we want Pylint to ignore it for now and will return to checking out alternatives after Pycon, because we expect there will be some better options by then.
I broke another linter with my linterignore by not putting a space where expected in a comment. Fixing that here.
Turns out there were TWO spaces the linter wanted that I didn't have! 🤣 This is what I get for reviewing PRs in the GitHub WebUI.
Please, Python Formatting, Have I Added Enough Whitespace Yet???? 😭
Now it's VERY terse, here's hoping this is enough to pass.
| @@ -3,7 +3,9 @@ | |||
| from abc import ABC, abstractmethod | |||
| from typing import Optional, TypedDict, Any, Callable, Union, Iterator | |||
| from urllib.parse import quote_plus | |||
| import imghdr | |||
|
|
|||
| # pylint: disable-next=deprecated-module | |||
| HTTP_X_FORWARDED_PROTO: false | ||
| run: | | ||
| mypy bookwyrm celerywyrm | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
This looks like all whitespace changes that might have been intro'd by my editor... I kinda think we should... not? It's commit history churn if so
There was a problem hiding this comment.
A bunch of linter changes that had been applied in between ended up bundled in here. We also ended up with some similar stuff in some of the security upgrades, but because we're using squash&merge I'm not super worried about it? We could get rid of it if you'd like, but I'm not super worried either way.
I miscounted characters, off by 5. This should actually be short enough now.
requests.get was missing a timeout; added a timeout of 15 as aligned to the timeout currently used in bookwyrm/utils/isni.py
Looks like I copypasted more than I needed to! Oops.
This reverts commit 1552dcf.
No description provided.