Limit jekyll site zip uploads to PR

[ignotus666]

I was wondering whether it might make sense to only zip and upload the site on pull request - to make it available to download and check - and just build the site as a check (no zipping and uploading) when the same PR is merged. Currently it's uploading 2 basically identical artifacts for every PR/merge.

[ann0see]

Yes. That would make a lot of sense. If the code is exactly the same, there’s no need to run the check twice.
BTW: jamulussoftware/jamulus#1921 (comment) is related

[ignotus666]

Shall I leave out only the site zipping and uploading or the build too? I suppose both - we could bundle them in a separate job with needs: and only trigger that job if on: pull_request.

Edit: no, a separate job won't work as it needs to build and zip the output of the previous steps, which haven't been merged anywhere... Adding if: ${{ github.event_name == 'pull_request' }} to the last three steps (build, zip, upload) would do it.

I'm not confident enough about my knowledge to poke my head into that thread, but couldn't you just make a list of the files you don't want to trigger the workflow with, using a negative pattern in paths? E.g.:

paths:
  - '!CONTRIBUTING.md'
  - '!README.md'
  - etc.

[ann0see]

Yes:

- '!LICENSE.md'
- '!README.md'
- '!.gitignore'
- '!.nojekyll'
- '!.remarkrc'
- '!.spellchecker.json'
- '!CONTRIBUTING.md'

…but with paths-ignore: and without the !.

So the build would not run if these files were changed. However, Peter disliked my proposal in the mentioned PR. I just linked it for your reference

[ann0see]

Shall I leave out only the site zipping and uploading or the build too?

Avoid as much as possible ;-).

[ignotus666]

Avoid as much as possible ;-).

Will do.

I just linked it for your reference

Yeah I know, hence why I wasn't going to showcase my ignorance there :)

[ignotus666]

Looking at how to optimise which steps are run and when, I scribbled down a table to visualise it better. I can't be bothered to make a pretty one in markdown so a photo will have to do:

For PRs and merges, there are basically three scenarios for each: changes to EN .md files, changes to .po files, and changes to anything else (REST in the table). At the far left are the steps that can happen - or not. For instance, the two scenarios in the last two columns mean that the workflow needn't even run at all.

This mainly applies to the 'next-release' branch as 'release' has its own workflow for push events.

The conditions for the PR scenarios are already met, but in order for the jekyll.yml script to meet those for merges, a bunch of 'if' conditionals would have to be added and possibly a rearrangement of where the file filter goes - maybe nearer the top to create a variable that other steps might need to pick up. I think it's doable though I haven't got round to it yet, but @ann0see I just wanted to know if you can see anything wrong with this approach.

No rush BTW.

[ignotus666]

As usual I was overthinking this... it's much simpler than I thought, though the table did help quite a lot. #589 makes it behave as in the table above.

[ann0see]

Ok. Trying to get understand and go through the table.

if there's a change to the English .md files and we are in a PR:

• We need to get the cache content or create it (that's clear)
• We need to run "update templates" to update the .po files for all languages. However, according to the table, you don't push it, but just build it and create the zip file. Reason you don't push it: de-clutter the commit history. In theory you could already push to the branch which the PR is based on, not right after the merge. The question is if this is better/worse.
• We need to run "create all targets" to "compile" the .md versions of the files.
  --> I think, although it's unrelated, that we still need to document what these scripts are for and when they need to be run in the header of the scripts (Update Installation-for-Windows.po #558 (comment)). At least, I'm still a bit confused about them.

If there's a change to the translation (= po files).

• Cache (understood)
• Don't update templates, since there was no change in the .md files --> no need to update the .po files and no need to commit and push them again
• However: we need to re-generate the .md files (since they do not exist, we need to do it on every run)
• Of course, upload and zip them

If there's a change to e.g. a picture on PR:

• No need to update templates (since we can 100% be sure that no English .md files are changed --> please verify that this is really the case. What happens if you change a picture and a .md file?)
• Since the .po files weren't changed, there's no need to push the .po files/md files; since the picture was changed, we still need to run the generation of .md files script and upload all the stuff. Random thought: Do you commit *.po and *.md instead of commit everything?

If some files are changed (as mentioned above), we don't even need to start the process.

If it's a merge commit (but not a direct commit to next-release):

... and we changed the english .md files, we can be sure that the .po files were already updated and we already had a successful build --> since we didn't run "push changes" on the PR, we need to update the po files and push them. No need to push .md files --> no need to generate them.
I think this could be handled differently (if you already push modifications to the branch in the PR. However this might be complicated and have an issue: what happens if we modify english .md files directly on next-release without a PR. This should trigger the whole build (both scripts run and changes get pushed)).

... and .po files were changed: We know that the Pull Request already has a .zip file with the live site. Therefore there's no need to do anything. The translation change is already "live" in the .zip file.

... and the rest was changed: As above. We know that the .zip file exists

I hope I got this right too. After thinking through it there's only one open point (don't or do push to the branch from which a new PR comes if english .md files were changed)

[ignotus666]

Yes, I think you've got it right. I'll try and address a few of the things you raise:

In theory you could already push to the branch which the PR is based on, not right after the merge. The question is if this is better/worse.

We could do that, but it would require the user enabling GH actions in their repo, wouldn't it? That was my assumption anyway.

please verify that this is really the case. What happens if you change a picture and a .md file?

Well, since a .md file has changed, the filter gets triggered and the templates get updated, etc. - the same as if the picture hadn't been involved. But since we're talking about a PR, nothing ever gets pushed by the script (that step is conditional on it being a push event). It it were a push event, the picture would already have been merged - the updated templates (.po files) get pushed after the push event that triggered the workflow.

what happens if we modify english .md files directly on next-release without a PR

Yes, this is something that's not covered and I'm not sure what could be done - other than 'forbid' directly editing branches (since it bypasses the review process) unless it's an inconsequential change. Still though, if you directly push a .md file change to next-release, it will trigger the update-templates script, and in turn the updated .po files will also get pushed, so no biggie. The only difference is that you won't get the zipped/uploaded site artifact. Serves you right for not doing it through a PR ;-)

I think, although it's unrelated, that we still need to document what these scripts are for and when they need to be run in the header of the scripts

Do you mean in the po4a scripts?

[ignotus666]

I think one thing that leads to confusion could be the use of the term "templates". Strictly speaking, the templates were the .pot files that were actually removed, but the term still stuck as in my mind the .po files are also a form of template (they're exact copies of what the .pot files were). But it might be clearer to just say ".po files" instead. I'd leave "targets" though as it is descriptive of what they are.

The po4a setup has gradually grown in complexity from a website maintainer's POV (hopefully not from an external contributor's POV - that was the whole point), so when I have the time, I was thinking it would be good to describe the whole process in detail, in a single document, so others can get a proper grasp of what's going on - mainly what all the scripts do and why. I'm sure there are still many things that can be improved, because at the end of the day I don't think I even qualify as an amateur when it comes to writing scripts - I just picked up the challenge and blindly ran with it. So if someone else with more knowledge ever finds the time to dig into it, such a document would be essential to disentangle it all.

[ignotus666]

A document explaining the po4a setup has been added to #589.

[ann0see]

Great! Thanks for this.

In theory you could already push to the branch which the PR is based on, not right after the merge. The question is if this is better/worse.
We could do that, but it would require the user enabling GH actions in their repo, wouldn't it? That was my assumption anyway.

Not sure about that. But I think if the action runs on the PR, and the user did not disable write access to maintainers, it might be possible to push directly.

please verify that this is really the case. What happens if you change a picture and a .md file?
Well, since a .md file has changed, the filter gets triggered and the templates get updated, etc. - the same as if the picture hadn't been involved.

That’s what you think happens. But did you test it? See the mentioned PR and comment by @pljones from above. I assume it’s different since we’re using another GH action here.

[ignotus666]

Not sure about that. But I think if the action runs on the PR, and the user did not disable write access to maintainers, it might be possible to push directly.

True. We could test to see if it that works, editing the script and making a PR. If it can be done, I think it'd be good because that way contributors can see what their translation looks like in .md format.

That’s what you think happens. But did you test it?

Yes, it's been tested and works as it should.