search

moodle-an-hochschulen / moodle-theme_boost_union

Theme Boost Union is an enhanced child theme of Boost which is intended, on the one hand, to make Boost simply more configurable and, on the other hand, to provide helpful additional features for the daily Moodle operation of admins, teachers and students.
GNU General Public License v3.0
66 stars 58 forks source link

Feature: Write a github action checking pull requests for use of non get_config() theme settings references #257

Closed danowar2k closed 9 months ago

danowar2k commented 1 year ago

This came up in a discussion. If Boost Union settings are set in Boost Union and child themes only implement new settings and use Union's settings in their own code, Boost Union should always use get_config() calls for its settings.

A github action checking this should check for usages of "$PAGE->theme->settings->" and warn accordingly to change this to get_config().

danowar2k commented 1 year ago

Okay, since github actions are detected and read on the default branch of a repository, I had to change the actions in master on my fork.

I've disabled the moodle-plugin-ci action temporarily to not run 1-hour-long tests every time.

I then added an action file for the PR check: https://github.com/danowar2k/moodle-theme_boost_union/blob/%23257-github-action-theme-settings/.github/workflows/check-pull-request.yml

This uses a marketplace action (https://github.com/JJ/github-pr-contains-action) and simply checks if the string "->settings->" is in the added pat of a pull request diff. I didn't use more than that string, because variable names could vary, and that part seems to be the only consistent part.

Sadly, it seems one cannot set a message to show in case of failures. But I haven't read all of github actions docs, so there may be some more general failure message variable.

I've tested it with a PR that contains it: https://github.com/danowar2k/moodle-theme_boost_union/actions/runs/4467755293

And with one that deletes it: https://github.com/danowar2k/moodle-theme_boost_union/actions/runs/4467783705

Any suggestions/requests regarding this are appreciated.

At the end of this I'll have to rebase my master again :-D

Links:

abias commented 9 months ago

Hi @danowar2k ,

many thanks for this very useful addition. I just went ahead and added the check to Boost Union's main Moodle-plugin-CI workflow instead of creating a dedicated PR workflow.

This approach triggers notifications saying Warning: ⚠️ Not a pull request, skipping PR body checks during normal pushes, but it allows the check to run alongside all other Moodle code checker checks which is preferable from my point of view.

In addition to that, I have added a section for to our reviewing wiki on https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests#additional-aspects-for-boost-union which covers the same topic now.

Cheers, Alex

danowar2k commented 1 month ago

Didn't know that was finished ^^