Skip to content
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

feat(templates): add workflow to update workflows from templates #244

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Oct 30, 2023

Demo run at https://github.com/st3iny/mail/actions/runs/6681157551/job/18155010149

Note: It fails as expected due to not being able to create the PR. The token secret will take care of that in a real repo.

Problems

  • Manual changes will be overwritten. Some of our apps/repos have slightly edited versions of templates.

@st3iny st3iny added the 2. developing Work in progress label Oct 30, 2023
@st3iny st3iny self-assigned this Oct 30, 2023
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I like it :)

@nickvergessen
Copy link
Member

The thing is, these are templates. Mail app (with it's stable branches) is the prime example where you will have a PR every time and you actually don't want the changes, e.g. stable25 was dropped already in june from https://github.com/nextcloud/mail/blob/main/.github/workflows/psalm-matrix.yml#L21 because 25 is covered by a stable branch. So since then every weekend you would have received a PR to readd it.

So we would need something like a "lock" file for the workflows and then it could be logged that a PR for a version was sent and upstream had no interest and we are not retrying it.

Atm I usually tried to include this to my version bump PRs, because I have to adjust some of the files there anyway, but it gets more and more complicated, so looking forward to a solution, but I guess it's just not that simple by default :P

@szaimen
Copy link
Contributor

szaimen commented Oct 30, 2023

I still think the workflow makes sense even in its current form. Each maintainer can now decide if the currently implementation is good enough for them or not. But the workflow will take care of any update to the template for possible improvements in the future

@szaimen szaimen requested a review from skjnldsv November 1, 2023 09:31
@nickvergessen
Copy link
Member

Idea how to solve: #244 (comment)

We could add a "random string"/"date+time" in the first comment block each file has, that has to be modified everytime the file is changed. This way we could just check if the "identifier" line matches:

  • if yes => don't send update
  • if it does not match => send update

An app like mail could then simply update the identifier line only and would be save from "downgrading" PRs every week.

@icewind1991
Copy link
Member

icewind1991 commented Nov 3, 2023

#248 and #249 should remove the need to update stable branches in the workflows.
Apps might have made other customizations to the templates though that this would overwrite.

Maybe the workflow can check if the workflow is changed from upstream and skip updating it when it is (and leave a note about it in the pr). Or event attempt to merge the change into the new template

@szaimen
Copy link
Contributor

szaimen commented Nov 5, 2023

What about being able to define the branches that the workflow runs against in a separate file per repo and the matrix then gets generates from that file? This would skip the need of having to change the workflow in order to adjust the branches that it runs against.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants