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

NEW Add merge-ups #2

Merged

Conversation

emteknetnz
Copy link
Member

Issue silverstripe/gha-merge-up#4

Also updates the logic for license

@GuySartorelli
Copy link
Member

Please add information about what these changes actually are - they're obviously going beyond what the purpose of the issue is. You mentioned something wasn't working as an off-hand comment in person so I assume this is mostly bug fixes? What were the bugs? What are you doing here and why?

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 11, 2023

I experienced loads of issues when running this for real. Some things I do remember going wrong:

  • Realising after creating heaps of PRs that we need keepalive.yml in any repo that has merge-up.yml in it
  • Creating the option "--update-prs" so that existing PRs were updated rather than having to close all the current open ones
  • Issues when there was a newer version of the automatically created branches and using "--update-prs"
  • There was some repo that had 2x license files in it
  • Needing to add gha-* modules
  • Outputting PHP errors to console while still showing which PRs were created
  • Special handling for weird repos - developer-docs, silverstripe-simple, cwp-watea-theme

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Once this PR has been merged, it'll need to be run again since the number generator implementation has changed. Best to wait until its merged so we don't have auto-generated PRs being merged and then recreated and merged and etc etc if this needs multiple rounds of review.

About the new "update-prs" option

This option is confusing to me. I would expect that this would update PRs if they exist, but still run on all modules whether there is a PR or not.
e.g. I run it once, and it creates some PRs. I then run it again, before the PRs have been merged, and it updates PRs that exist and also runs against any modules that were missed the first time around.

The above seems like a far more likely scenario than wanting to only run the scripts against repos which already have PRs. What is the intended scenario for the current implementation, where it will skip repositories which don't have PRs?

Note that I haven't reviewed the code changes in update_command.php related to that new option pending a response to this question as I really don't understand the intended use case.

funcs_scripts.php Outdated Show resolved Hide resolved
funcs_scripts.php Outdated Show resolved Hide resolved
funcs_utils.php Outdated Show resolved Hide resolved
funcs_utils.php Outdated Show resolved Hide resolved
funcs_utils.php Outdated Show resolved Hide resolved
scripts/cms5/merge-ups.php Show resolved Hide resolved
scripts/cms5/merge-ups.php Show resolved Hide resolved
update_command.php Outdated Show resolved Hide resolved
update_command.php Outdated Show resolved Hide resolved
update_command.php Show resolved Hide resolved
@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 14, 2023

I would expect that this would update PRs if they exist, but still run on all modules whether there is a PR or not.

Yeah that seems like a really nice use of this new flag. Previously intention was to use copy-paste a list of repos run into the --exclude flag if things broke half-way through, though you could simply use --update-prs instead which seems like it would be much nicer. --update-prs was only added late after I realised I needed to add a bunch of keepalive.yml files to existing PRs.

That said this PR is already very convoluted and implementing this new would further convoluted it - I've split this off as a new card

funcs_scripts.php Outdated Show resolved Hide resolved
scripts/cms-any/editorconfig.php Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Remaining changes (I forgot to hit start review):

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks okay. There are some follow-up cards created for things that weren't quite optimal but can be dealt with later.

@GuySartorelli GuySartorelli merged commit f70f588 into silverstripe:main Aug 14, 2023
@GuySartorelli GuySartorelli deleted the pulls/main/merge-ups branch August 14, 2023 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants