-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use Github Actions instead of Travis #121
base: master
Are you sure you want to change the base?
Conversation
try to use python 3 module - when available add fallback to just the lektor command if it fails
+ Because @xperimental deleted this + Because multiple checks are better than no check (in case of travis is slow again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @DO1JLR. We had thought about replacing Travis with Github Actions during the weekend already but deemed it too much overhead over the actual goal of updating some data on the websites.
I think it is a nice start, we can build on top of this, depending on whether we want to switch to Github Actions. I've already left you some comments on the existing state, but if we want to switch fully this still needs some work.
@@ -1,3 +1,4 @@ | |||
# Information for Github sponsor button | |||
|
|||
github: [do1jlr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided that it is probably the best solution to link to the contribution pages of both websites instead of private accounts to make things more transparent (see #119).
Can you remove your private account again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Wir" hatten das bei der einführung des spenden-buttons noch anders gehandhabt.
U.a. waren Teile des Vorstands damit einverstanden dies darin zu haben.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grundsätzlich finde ich es besser, wenn die Spenden alle an eine zentrale Stelle gehen, das war schließlich der Grund warum wir die Sache mit dem Förderverein überhaupt angefangen haben, dass es nicht mehr so ein durcheinander gibt. Dachte eigentlich auch dass das deine Meinung ist nach der Nachricht neulich.
Nichtsdesdotrotz hat die Änderung hier auch nix mit der Anderen zu tun, muss also eh in einen anderen PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Du hast das auch alles in einem PR mit reingeschummelt btw
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.5, 3.6, 3.7, 3.8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the main purpose is whether the website still works, I don't think we need to test that many Python versions. The most recent stable is probably enough. Looking at my system this seems to be the 3.7 branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Und da wären wir schon bei Problemen.
Du magst altertümliche python versionen wie 3.7 verwenden. Ich verwende 3.8. Travis nimmt 3.6. Andere womöglich noch älter...
Lass uns doch python probleme erkennen in dem wir alle gängigen versionen testen!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wir schreiben hier ja keine Bibliothek für Python, bei der wir ständig testen müssen ob die noch mit allen Versionen kompatibel ist. Prinzipiell würde es Sinn ergeben das mit der Version laufen zu lassen die von den Meisten die die Website bearbeiten verwendet wird, was momentan vermutlich die 3.7 ist, aber meinetwegen können wir auch schon die 3.8 nehmen, der Standard wird sich ja wahrscheinlich eh bald ändern, wenn der Support für 3.7 dieses Jahr abläuft.
Also sagen wir einfach schon 3.8
.
Nachtrag: Ich sehe auch gerade das 3.8 noch garnicht auf der Lektor-Seite als "kompatibel getestet" steht. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also eine offiziell nicht supportete pythonversion? Als single source of truth? sicher?
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
- name: Cache lfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step name seems to be a duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ein update von pip tut nicht weh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicht die Zeile darüber, sondern diese. Der Name des Steps ist der Gleiche wie der darunter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dann markiere sie doch bitte korrekt.
Protipp: Es gehen auch multiline-markierungen!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ja gut, das sollte - wie man sieht - lektor heißen.
sudo apt-get install git-lfs | ||
git lfs pull | ||
- name: install lektor | ||
run: make install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sudo make install
(also see comment on Makefile).
@@ -7,24 +7,35 @@ all: build | |||
.ONESHELL: | |||
install: | |||
if hash apt-get 2>/dev/null; then | |||
sudo apt-get update -qq >/dev/null && sudo apt-get install -qq apt-utils imagemagick python3-pip python3-setuptools gcc | |||
sudo apt-get update -qq >/dev/null && sudo apt-get install -qq apt-utils imagemagick python3-pip python3-setuptools gcc git-lfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefiles which modify the system configuration usually do not have the sudo
calls in them. Instead you are supposed to start make
with sudo (or your system's equivalent) yourself to avoid surprising changes to user's systems. For make install
this is usually the case. I'd prefer it if you could remove the sudo
calls from the Makefile to avoid people accidentally modifying their system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: Couldn't we also install lektor from the package repositories instead of installing it globally with pip
? I have the feeling that this might have unexpected behaviour for people actually using Python on their systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besser nicht.
Wenn wir die package repositories
verwenden haben wir mal wieder potentiell komplett unterschiedliche lektor versionen oder im zweifelsfall einfach gar keine weil nicht im offiziellen repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefiles which modify the system configuration usually do not have the
sudo
calls in them. Instead you are supposed to startmake
with sudo (or your system's equivalent) yourself to avoid surprising changes to user's systems. Formake install
this is usually the case. I'd prefer it if you could remove thesudo
calls from the Makefile to avoid people accidentally modifying their system.
Das könnte man machen...
ABER die sudo calls sind schon vor diesem commit drinen und gehören doch eher in einen neuen Pull-Request! Daher nein.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naja, du änderst ja sowieso schon einige Sachen hier in der Datei, damit das mit der neuen CI gut funktioniert, da kann man diese Änderung auch gleich mitnehmen, das ist denke ich themenverwandt.
@@ -0,0 +1,39 @@ | |||
name: lektor check | |||
on: [push, pull_request] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably only do this for pull-requests as branches could be quite "unready" and we already had some issues with running into quota limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welche Issues hattest du denn?????
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die FFBsee orga hat nachwievor Probleme innerhalb des LFS Quota zu bleiben. Ich hab gesehen dass du versucht hast das dadurch zu lösen dass die Daten zu dem gitea bei see-base ausgelagert werden, aber das ist auch nur ein Workaround, da dann jeder einen Account bei see-base braucht und das Repo nun von zwei Servern abhängig ist.
Ich hab noch nicht nachgeschaut wie lange die Änderung mit LFS schon her ist, aber momentan ist das Quota noch stark überzogen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Da darfst du lange warten.
Das LFS Quota wird so schnell auch nicht wieder frei werden bis microsoft das umstellt!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dann schau nochmal nach und dann wirst du selber feststellen das diese aussage inhaltlich nicht ganz zutreffend ist!
@@ -1,3 +1,4 @@ | |||
# Information for Github sponsor button | |||
|
|||
github: [do1jlr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grundsätzlich finde ich es besser, wenn die Spenden alle an eine zentrale Stelle gehen, das war schließlich der Grund warum wir die Sache mit dem Förderverein überhaupt angefangen haben, dass es nicht mehr so ein durcheinander gibt. Dachte eigentlich auch dass das deine Meinung ist nach der Nachricht neulich.
Nichtsdesdotrotz hat die Änderung hier auch nix mit der Anderen zu tun, muss also eh in einen anderen PR.
@@ -0,0 +1,39 @@ | |||
name: lektor check | |||
on: [push, pull_request] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Die FFBsee orga hat nachwievor Probleme innerhalb des LFS Quota zu bleiben. Ich hab gesehen dass du versucht hast das dadurch zu lösen dass die Daten zu dem gitea bei see-base ausgelagert werden, aber das ist auch nur ein Workaround, da dann jeder einen Account bei see-base braucht und das Repo nun von zwei Servern abhängig ist.
Ich hab noch nicht nachgeschaut wie lange die Änderung mit LFS schon her ist, aber momentan ist das Quota noch stark überzogen.
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.5, 3.6, 3.7, 3.8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wir schreiben hier ja keine Bibliothek für Python, bei der wir ständig testen müssen ob die noch mit allen Versionen kompatibel ist. Prinzipiell würde es Sinn ergeben das mit der Version laufen zu lassen die von den Meisten die die Website bearbeiten verwendet wird, was momentan vermutlich die 3.7 ist, aber meinetwegen können wir auch schon die 3.8 nehmen, der Standard wird sich ja wahrscheinlich eh bald ändern, wenn der Support für 3.7 dieses Jahr abläuft.
Also sagen wir einfach schon 3.8
.
Nachtrag: Ich sehe auch gerade das 3.8 noch garnicht auf der Lektor-Seite als "kompatibel getestet" steht. 🤷♂️
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
- name: Cache lfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicht die Zeile darüber, sondern diese. Der Name des Steps ist der Gleiche wie der darunter.
@@ -7,24 +7,35 @@ all: build | |||
.ONESHELL: | |||
install: | |||
if hash apt-get 2>/dev/null; then | |||
sudo apt-get update -qq >/dev/null && sudo apt-get install -qq apt-utils imagemagick python3-pip python3-setuptools gcc | |||
sudo apt-get update -qq >/dev/null && sudo apt-get install -qq apt-utils imagemagick python3-pip python3-setuptools gcc git-lfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naja, du änderst ja sowieso schon einige Sachen hier in der Datei, damit das mit der neuen CI gut funktioniert, da kann man diese Änderung auch gleich mitnehmen, das ist denke ich themenverwandt.
No description provided.