forrtproject / forrtproject.github.io

FORRT Website
https://forrt.org
Other
30 stars 18 forks source link

Exempt content folder from review requirements? #136

Open LukasWallrich opened 1 month ago

LukasWallrich commented 1 month ago

Ideally, project leads would be able to make minor content changes without waiting for approval - could the content folder be exempted from the requirement to have review by someone else?

If that does not work, maybe it should live in another repository (so that we can also have fewer people able to edit the more technical parts of the webpage)?

DAKiersz commented 1 month ago

This is a good suggestion, I believe we need to atomise the CODEOWNERS file a bit more and look into rulesets that will help enforce these.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets

bethaniley commented 1 month ago

Hi Lukas! Team leads should absolutely have control over content changes. Although perhaps keeping one review might still be helpful for identifying typos etc.?

I explained the reason for two reviewers in another issue previously, here's what I said:

The reason we changed this to two reviewers was to prevent changes which break the website. A lot of previous approvals have been "looks good to me" approvals, in the sense that the person reviewing doesn't seem to check the code, or even outright admits that they don't know whether the untested code will work. As part of the IOI grant, we reviewed this and identified it as a potential weak point for future releases.

The vision for the two-person review system is that there would be:

  1. One technical reviewer - someone who understands the code and can actually check it. That is generally performed by myself, Dominik K., Sam G., Richard D., or Lukas W.
  2. One conceptual reviewer - does the proposed change make sense for FORRT's mission/vision? This generally comes from Flavio A., but can be done by anyone in FORRT's management.

You can think of this as analogous to peer review of a journal article. If they don't understand the conceptual framing and methodology, are they actually reviewing the paper?

TL;DR: it's primarily for safeguarding technical changes, but does seem largely unnecessary for content changes.

That being said, we need to clearly identify what's a content change and what isn't. For example, the recent Glossary changes were within the /content/ directory, but were technical changes (in the sense of changes to scripts which did not deploy as expected).

Potentially we could change the settings so that changes to markdown and image files only need one review, whereas changes to other file types need two. Looks like Dominik has identified a method to do that!

(Note: Edited for clarity & to merge two identical issues)

LukasWallrich commented 1 month ago

I like the idea to separate this by file type rather than folder (as that also makes it easier to add static files)