Merge-powered recipe update system #845
Conversation
|
It's Christmas before Christmas! 💝 |
Did you consider using git instead of maintaining this directory? cloning the recipe repo locally and using git to checkout the tree-ref? Not saying this is would be better. This is just what I had in mind :) |
|
I just read some part and it an amazing work! 👏 |
That definitely seems like a valid approach. I guess I was trying to (A) minimize the amount of work needed on the user's local computer (e.g. git cloning the recipes repo) and (B) loading the "old" recipe data in the exact same way as normal (recipes are normally loaded from JSON files... so why not do the exact same thing to load the old recipes)? So, I think my approach for that part is the correct one. However, that represents just a small part of the PR: if we needed to change that, it wouldn't be a huge thing. |
|
With longer URLs to cache, we might hit #830 soon. |
I tested this out - here's an example cache key for an archive:
That's length 108. So we have 35 more chars to work with, which seems unlikely to hit. UNLESS someone uses a non-Github endpoint, in which case the current I've just pushed a little tweak where, if the key is longer than 140 (this lives a little wiggle room for eCryptfs), we |
Nice thanks. While you're on this topic, I know we have an issue right now related to files that are removed from recipes. Eg when |
Yup! It catches those! You can even see it at the end of the video in the PR description: a I don't see a specific issue or PR about that, but I know it has been an issue (there is #706, but it's about |
|
I've tested on Windows updating a few recipes and it worked without issues for me! |
|
Did you consider submitting against |
or just a longer package name than |
I was hoping you wouldn't ask ;). But strictly speaking, I CAN change to that if you want. It would mostly involve removing types and a few other things. Annoying, but doable. WDYT? Comments so far have been addressed. Only 1 TODO left in the code, pending the full |
|
Haha, then please target |
|
The I'll re-work this for the 1.x branch, but just because you asked nicely ;) |
weaverryan
force-pushed
the
recipe-update
branch
from
fde094f
to
57e6b5d
Compare
weaverryan
force-pushed
the
recipe-update
branch
2 times, most recently
from
816483d
to
2cd5441
Compare
|
I've setup an outdated project to use your fork+branch of symfony/flex and tried to run A quick Google gave me some suggestions. I think you should probably add In src/Update/RecipePatcher.php: @@ -107,6 +107,7 @@ class RecipePatcher
try {
$this->execute('git init', $tmpPath);
+ $this->execute('git config commit.gpgsign false', $tmpPath);
$this->execute('git config user.name "Flex Updater"', $tmpPath);
$this->execute('git config user.email ""', $tmpPath); |
|
The next thing I'm running into is that I've got the habit of removing those This happened when running recipe update for After Great job @weaverryan ! |
|
@7ochem THANK YOU for thoroughly testing this - that awesome!
I've added your suggested fix for this - thank you!
This was a bug I introduced in the latest feature I added - very good catch! It's now fixed - it will work without needing to So, tests are still green and all comments have been addressed! |
|
One more thing... When trying this out, I had changed my composer.json (and composer.lock) file because I had to target your fork and this branch, so I had uncommitted changes in those files when running Maybe you could show a warning at the start that there are uncommitted changes in files and advise to commit any changes before running |
Done! |
There was a problem hiding this comment.
A very quick test run of this PR (and added some suggestions about the command class). I love how easy it is to get the references to the changes and to fix conflicts with local modifications! Great job!
It comes with a small price of requiring to have a clean git index, etc. But that small price is worth the benefits this gives us.
One thing that might be improved is the final git state after running this command. As the changes are applied, they are now part of the git index. But the symfony.lock changes aren't. I would suggest to either make sure everything modified by the command is part of the index, or nothing.
|
Thank you for the thoughtful review Wouter!
To make sure I understand, are you referring to the fact that the |
weaverryan
force-pushed
the
recipe-update
branch
from
c257547
to
4e8afca
Compare
I'm sorry for the confusion, this is way over my head for flex internals, but I'm sure your answer will feed ideas to other flex contributors :) What I was talking about was the git index. After running I would love if either all changed files and |
Oh, of course! Nice idea - I've just made that change :) |
|
Having to select a package in the list doesn't feel like a real choice. Aka how can I know which package I want to update? (The simple way to take my concern into account is to set 0 as the default choice in the list of choices and tell ppl to run the command as many time as needed, but I'm wondering if we can do better) |
|
The good thing about doing it one by one is the ease of review: the diff is smaller and easier to understand I suppose. |
|
You’re not the first person to ask about updating all of them at once. I agree with Fab that it’s nice to review them one-by-one, including the CHANGELOG that’s generated for each. If we were to update all at once, from a practical perspective, I think we would need to commit automatically because the patch system needs the working tree and index to be clean… though i can’t remember the exact reason why (I did add a check for this on purpose). So I’d like to default to the first choice and take the easy way out (just default the 0 choice) - but I’m open to either, which would include either (A) committing for the user or (B) more likely, investigating if we can apply changes to a dirty tree. |
From a usage point of view, that last point is very interesting (I also preferred applying on a dirty tree when testing the current feature). But to keep the scope small, I would do it outside this big PR :) |
|
New look, with the default :)
|
There was a problem hiding this comment.
LGTM!
Some note about the changelog that is displayed at the end of an update:
- because it relies on calls to api.github.com, it won't be compatible with private recipes (but we don't have to care until someone sends a PR to improve this), but the update itself will still work if I understood correctly since updates rely only on the JSONs.
- I'd prefer if we could use one-liners:
* #1012 - Add missing return type (@dreadnip)
https://github.com/symfony/recipes/pull/1012
could be
* #1012 - Add missing return type - https://github.com/symfony/recipes/pull/1012
we could even make the PR number clickable and hide the full URL:
* Add missing return type (#1012)
- it'd be nice to display the same changelog when running the
recipes foo/barcommand (this command is a bit outdated now :) )
For what's it worth: I don't think all shells have support for this |
I think the vast majority does. Let's make things work best for most people. I agree that the current changelog is quite "heavy". |
|
Changelog shortened :)
Correct! As long as the private recipe API implements the same structure as the main one (the same JSON files, etc), then the update will work. And (as you know) https://github.com/symfony-tools/recipes-checker has the tools that private recipe API's can use (the |
|
Thank you @weaverryan. |
…rryan) This PR was submitted for the 5.4 branch but it was squashed and merged into the 4.4 branch instead. Discussion ---------- Adding details about new recipes:update command See: symfony/flex#845 The `versionadded` version may still change if we decide to put this into the 1.x branch. Commits ------- 11d87ad Adding details about new recipes:update command
Hi!
tl;dr; Updating recipes was a pain. Now its easy: the new
recipes:updatecommand performs a smart "patch" to your project of what actually changed in the recipe.If you run simply
composer recipes:update, then it will ask you what to update, from a list of only the out-dated recipes (thanks to @shadowc for the idea!):As you can see, if there are conflicts, you fix them like normal git conflicts. In addition to the unit tests, I've tried this on a fairly out-dated project and it worked brilliantly. This was NOT possible until @nicolas-grekas open sourced the Flex server - so a HUGE thanks 🎆
How It Works
A) Whenever
symfony/recipes(or contrib) has a new commit, a GitHub action already stores each recipe as JSON, which becomes the "recipe API": https://github.com/symfony/recipes/tree/flex/main. A change to that action (symfony/recipes#1037) and the recipes-checker (symfony-tools/recipes-checker#2) will add an "archived/" directory that contains EVERY version of every recipe. You can see an example in my fork: https://github.com/weaverryan/recipes/tree/flex/main/archivedB) When you run
recipes:update <package/name>, we fetch the "original" recipe and "new" recipe and then pass both to each "configurator". Its job is to say what files would exist and what they would look if the "original" recipe were installed now and if the "new" recipe were installed now.C) We use these "original files" and "new files" to generate a patch file.
D) We apply that patch file to the user's actual project. There are a few tricks here, like temporarily inserting the git "blob" for what the "original" version of the file would look like. That allows for a 3-way merge.
So, while it was a bit of work, it's a fairly straightforward process. It makes keeping your recipes up to date both easy and rewarding.
TODOS
Downloaderin this PR to remove 2x TODOs that the above PR's will enablearchived/directory is populated for bothrecipesandrecipes-contrib- see Adding a command to generate the complete archived recipe history for a repository symfony-tools/recipes-checker#3 and Add callable action to repopulate the recipe archives recipes#1038Cheers!