github:pr command
#1179
github:pr command
#1179
Conversation
|
Download the artifacts for this pull request: |
|
Very cool. Does it support auto discovery i.e. adding |
I currently replace only paths. |
|
See https://github.com/vaimo/composer-patches#usage-embedded-metadata Basically the package to patch and the level of directories to remove is added to the patch file. Together with the search path defined once in the composer.json, the patches should just work without any additional manual changes, if this is implemented |
|
But looking closer at the path replacement you are doing, this might not be needed. We usually use the diff as is from github and set the level and package accordingly. |
cmuench
force-pushed
the
feature/github-patch-command
branch
2 times, most recently
from
21de1ad
to
96bfd83
Compare
| public static function create(string $diffContent): string | ||
| { | ||
| $diffContent = self::processAppCode($diffContent); | ||
| $diffContent = self::processAppDesign($diffContent); |
There was a problem hiding this comment.
You'll probably still need to handle magento/framework and magento/magento2-base directories as well?
The latter doesn't need adjustments in the path I think, but the first one needs to be stripped from lib/internal/Magento/Framework
Some examples:
| ->addOption('diff', null, InputOption::VALUE_NONE, 'Raw diff download') | ||
| ->addOption('json', null, InputOption::VALUE_NONE, 'Show pull request data as json') | ||
| ->setDescription('Download patch from github merge request <comment>(experimental)</comment>'); | ||
| } |
There was a problem hiding this comment.
Would be nice to have an option to specify to which directory it should write the patch file(s)
| } | ||
|
|
||
| $output->writeln(sprintf('<info>Patch file created:</info> <comment>%s</comment>', $filename)); | ||
| } |
There was a problem hiding this comment.
Does this method handle generating multiple patch files, for each module separately? It's a bit hard to check by just reading the code, but I think it doesn't?
For example: magento/magento2#34360, should (in my opinion, but some people disagree) be splitted in 2 patches, for:
magento/magento2-basemagento/module-quote-graph-ql
There was a problem hiding this comment.
Here's maybe a better example: magento/magento2#27850
Involves:
magento/framework-message-queue(fun edge case for your code, why Adobe didn't put this inmagento/frameworkis beyond me, but hey ...)magento/magento2-basemagento/module-asynchronous-operationsmagento/module-message-queue
There was a problem hiding this comment.
Right. PR is not ready to merge and some stuff is currently not handled. I guess we have also to manage the language packs here.
The magento/framework-message-queue package was initially part of the Enterprise Edition. Maybe that's why it's not part of the framework.
Is splitting in different patch files a good idea?
There was a problem hiding this comment.
Splitting is in my eyes necessary because you need to specify to which module a patch should apply, and if you specify from one module that some patch should apply to another module it will not work if you change the paths.
Unless you don't change the paths and then specify a very high patch level (I think it's 4 for Magento?).
Also found this super old discussion from years ago about this: cweagans/composer-patches#76 (comment)
I'd say, give it a try with a complex PR like I specified above and see how well it works.
There was a problem hiding this comment.
If someone uses ": "cweagans/composer-patches" then... we e.g. do not use it in Magento projects. The Quality Patches are also not splitted.
There was a problem hiding this comment.
My current plan is not to split. Splitting sounds for that we will face a lot of possible issues.
There was a problem hiding this comment.
No problem, looking through the code again, it looks like I may have misunderstood what you are tying to do.
So you can ignore this comment 🙂
Out of curiosity: what method of applying patches do you use? I think that the majority of Magento devs use either cweagans/composer-patches or vaimo/composer-patches (and both probably can work with the format of patches you are creating here)
There was a problem hiding this comment.
At netz98 we have an own patch repository with a apply-patch.sh script (in the patch repo). This is then called via post-install-script/post-update-script in the composer.json.
Super simple solution.
Similar to the Quality Patches of Adobe.
For n98-magerun2 we should find a useful way for everyone. We use cweagans/composer-patches in other PHP projects, too. For Magento projects I do not like this splitting. IMHO it's a over-complication.
This does not mean that we should not support it (if it's possible).
I have not used the vaimo/composer-patches package. On the first look it looks for me that a standard (not splitted patch) can be used.
cmuench
force-pushed
the
feature/github-patch-command
branch
from
96bfd83
to
cff6763
Compare
cmuench
force-pushed
the
feature/github-patch-command
branch
from
cff6763
to
50bbd63
Compare
|
@hostep The Message Queue edge case is now handled and we have support for |
cmuench
force-pushed
the
feature/github-patch-command
branch
from
9c53dc9
to
33522fd
Compare
Magerun pull-request check-list:
Summary: Add a new command to work with Github PRs
Features of the new command