Magento Quality Patches - improvement of dev experience #61

viktym · 2020-07-21T20:46:55Z

Description

MC-36033 Split ece-patches and magento-patches application
MC-35802 Dev experience - Implement a service to determine which patch is causing a conflict
MC-35824 Dev experience - Adapt tool for working with git-instance Magento

Preconditions for test scenarios

Check if your GitHub token has access to https://github.com/magento-mpi/magento-cloud-patches.git, https://github.com/magento/quality-patches.git, https://github.com/magento-mpi/ece-tools.git
Add to composer.json

 "repositories": {
        "repo": {
            "type": "composer",
            "url": "https://repo.magento.com"
        },
        "mcp": {
            "type": "vcs",
            "url": "https://github.com/magento-mpi/magento-cloud-patches.git"
        },
        "mqp": {
            "type": "vcs",
            "url": "https://github.com/magento/quality-patches.git"
        }
    },
    "require": {
        "magento/magento-cloud-patches": "dev-git-based as 1.0.99",
        "magento/quality-patches": "dev-MC-TEST as 1.0.999"
    },

run 'composer update'
commit changes

Testing scenario 1 - Applying optional quality patch on a local and remote Cloud instance

run './vendor/bin/ece-patches status'
Expected: all patches have status 'Not applied'
Add to .magento.env.yaml

 stage:
    build:
        QUALITY_PATCHES:
            - MCTEST-1001

run on local instance './vendor/bin/ece-patches apply'
run on local instance './vendor/bin/ece-patches status'
Expected: all patches with Type 'Required' and Optional patch MCTEST-1001 have the status 'Applied'
Push local changes to remote branch
After redeploying login on a remote instance by SSH
run on remote instance './vendor/bin/ece-patches status'
Expected: all patches with Type 'Required' and Optional patch MCTEST-1001 have the status 'Applied'
Expected: /var/log/patch.log file contains

INFO: Patch MCTEST-1001 has been applied {"file":"/app/vendor/magento/quality-patches/src/../patches/MCTEST-1001__TEST_changes__2.3.5_ce.patch"}
INFO: Patch MCTEST-1001 has been applied {"file":"/app/vendor/magento/quality-patches/src/../patches/MCTEST-1001__TEST_changes__2.3.5_ee.patch"}

Testing scenario 2 - test ece-patches 'apply' and 'revert' with QUALITY_PATCHES: '*' on local Cloud instance

on local instance run './vendor/bin/ece-patches revert'
on local instance run './vendor/bin/ece-patches status'
Expected: all patches have status 'Not applied'
Set to .magento.env.yaml

 stage:
    build:
        QUALITY_PATCHES:
            - '*'

run on local instance './vendor/bin/ece-patches apply'
run on local instance './vendor/bin/ece-patches status'
Expected: all patches have the status 'Applied'
on local instance run './vendor/bin/ece-patches revert'
on local instance run './vendor/bin/ece-patches status'
Expected: all patches have the status 'Not Applied'

Testing scenario 3 - test magento-patches 'apply' and 'revert' on local Cloud instance

on local instance run './vendor/bin/magento-patches apply MCTEST-1001'
on local instance run './vendor/bin/magento-patches status'
Expected: patch MCTEST-1001 has status 'Applied', other patches have status 'Not applied'
run on local instance './vendor/bin/magento-patches revert MCTEST-1001 '
run on local instance './vendor/bin/magento-patches status'
Expected: patch MCTEST-1001 has status 'Not Applied',
on local instance run './vendor/bin/magento-patches apply MCLOUD-5650 MAGECLOUD-5069'
on local instance run './vendor/bin/magento-patches status'
Expected: patch MCLOUD-5650 MAGECLOUD-5069 has status 'Applied', other patches have status 'Not applied'
on local instance run './vendor/bin/magento-patches revert --all'
on local instance run './vendor/bin/magento-patches status'
Expected: all patches have the status 'Not Applied'
on local instance run './vendor/bin/magento-patches apply' without arguments
Expected: error is displaying: 'Not enough arguments (missing: "list_of_patches").'
on local instance run ./vendor/bin/magento-patches apply '*' and check result with './vendor/bin/magento-patches status'
Expected: all patches have the status 'Applied'
on local instance run './vendor/bin/magento-patches revert' without arguments
Expected: nothing happened, there are no patches to revert
on local instance run './vendor/bin/magento-patches status'
Expected: all patches still have the status 'Applied'

Testing scenario 4 - work on Magento git-instance

Clone Magento 2.3.5 using git tag 'git clone --depth 1 --branch 2.3.5 https://github.com/magento/magento2ce.git'
Add to composer.json

 "repositories": {
        "repo": {
            "type": "composer",
            "url": "https://repo.magento.com"
        },
        "mcp": {
            "type": "vcs",
            "url": "https://github.com/magento-mpi/magento-cloud-patches.git"
        },
        "mqp": {
            "type": "vcs",
            "url": "https://github.com/magento/quality-patches.git"
        }
    },
    "require": {
        "magento/magento-cloud-patches": "dev-git-based as 1.0.99",
        "magento/quality-patches": "dev-MC-TEST as 1.0.999"
    },

run 'composer update'
run './vendor/bin/magento-patches status'
Expected: see the table with available patches for Magento 2.3.5. All patches have status 'Not applied'
run './vendor/bin/magento-patches apply MCLOUD-5837' and check status './vendor/bin/magento-patches status'
Expected: MCLOUD-5837 has the status 'Applied'
run './vendor/bin/magento-patches revert MCLOUD-5837' and check status './vendor/bin/magento-patches status'
Expected: MCLOUD-5837 has the status 'Not applied'

Release notes

For user-facing changes, add a meaningful release note. For example, see Magento Cloud Patches release notes.

Associated documentation updates

Add link to Magento DevDocs PR or Issue, if needed.

Contribution checklist

Pull request has a meaningful description of its purpose
Pull request introduces user-facing changes and includes meaningful updates for any required release notes and documentation changes
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)

…agento - Added support of magento/magento2ce and magento/magento2ee installed from git tag

…ch is causing a conflict

shiftedreality · 2020-07-23T15:19:53Z

src/Command/Apply.php

-                InputOption::VALUE_OPTIONAL,
-                'Is git installation',
-                false
+                self::ARG_LIST_OF_PATCHES,


This is BC change, it should not go into patch version release

It's a new command - ./magento-patches apply,
./ece-patches apply remains without changes

@shiftedreality or you mean removing of --git-installation option?
Previously we discussed this with Bohdan and Olexandr, agreed that we can just not call ./ece-patch apply command from ece-tools in case of git installation

shiftedreality · 2020-07-23T15:21:58Z

composer.json

@@ -14,6 +14,7 @@
        "symfony/dependency-injection": "^3.3||^4.3",
        "symfony/process": "^2.1||^4.1",
        "symfony/proxy-manager-bridge": "^3.3||^4.3",
+        "symfony/yaml": "^3.3||^4.0",
        "monolog/monolog": "^1.16",
        "magento/quality-patches": "^1.0.0"


This causes circular dependency and must be moved to another package

The packages have a dependency on each other and the composer resolves them without issues. Why it should be moved?

Having circular dependency is never good idea, in some cases, it lead to neverending dependency resolving and other problems. This may be either included in ECE-Tools or MCC

And circular dependency consieded as "bad smell" from Architecture Design standpoint

Having circular dependency is never good idea, in some cases, it lead to neverending dependency resolving and other problems. This may be either included in ECE-Tools or MCC

I agree with you, in case of recursive read it can lead to neverending resolving.
In the current case we A => B and B => A. Composer resolves it well.
Otherway, if we move dependency on quality-patches to ece-tools, it will be impossible to install magento-cloud-patches from the composer separately, as described in docs
P.S. It will be possible, but impossible to use.

"magento/quality-patches" dependency was moved to suggest section
@shiftedreality please review

shiftedreality · 2020-07-23T15:26:38Z

src/ApplicationEce.php

+    protected function getDefaultCommands()
+    {
+        return array_merge(parent::getDefaultCommands(), [
+            $this->container->get(Command\ApplyEce::class),


It's better to move the classes under sub-namespace Command\Ece\Apply

Make sense, in progress ... thanks

@shiftedreality fixed, please check

shiftedreality · 2020-07-23T17:16:48Z

bin/ece-patches

@@ -4,7 +4,8 @@
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
+define('IS_CLOUD', true);


This is redundant since ApplicationEce already Cloud-aware

Yes, but there is CloudCollector service that is used by both applications and we need to define what is the environment to set patch type optional or required https://github.com/magento-mpi/magento-cloud-patches/blob/65837e0a187dfa282e47be67c7dc1cd349643709/src/Patch/Collector/CloudCollector.php#L94

shiftedreality · 2020-07-23T17:20:37Z

src/Command/Process/ApplyLocal.php

            } catch (ApplierException $exception) {
+                $this->printInfo($output, 'Error: patch conflict happened');


Use the color tags instead of status, <error></error>

It also must return corrent non-zero exit code

Use the color tags instead of status, <error></error>

Will be fixed, thanks

It also must return corrent non-zero exit code

It throws RuntimeException which leads to non-zero exit code
https://github.com/magento-mpi/magento-cloud-patches/blob/65837e0a187dfa282e47be67c7dc1cd349643709/src/Command/Process/ApplyLocal.php#L101

@shiftedreality fixed, please check

shiftedreality · 2020-07-23T17:24:52Z

src/Command/Revert.php

     */
-    const ARG_QUALITY_PATCHES = 'quality-patches';
+    const ARG_LIST_OF_PATCHES = 'list_of_patches';


Cloud Tools use hyphens instead of underscores

Will be fixed, thanks

@shiftedreality fixed, please check

shiftedreality · 2020-07-23T17:28:50Z

src/Environment/Config.php

+    /**
+     * Variable to define a Cloud environment.
+     */
+    const ENV_VAR_CLOUD = 'MAGENTO_CLOUD_PROJECT';


This is not needed since you have separate entry point for Cloud

The reason is that Status command is common for ece-patches and magento-patches. This command shows a table with patch type. I believed that is good to show all Clod patches as required even if the customer runs './magento-patches status' on the Cloud environment. Otherway cloud patches will display as optional. Does it make sense?

- fixes after CR

- bump version

- fix table render for Symfony < 4

andriyShevtsov · 2020-07-27T02:54:22Z

QA approved

MCLOUD-9275: Cloud Tools Release

viktym added 2 commits July 21, 2020 15:39

MC-35824: Dev experience - Adapt tool for working with git-instance M…

9d25311

…agento - Added support of magento/magento2ce and magento/magento2ee installed from git tag

MC-35802: Dev experience - Implement a service to determine which pat…

d6081b6

…ch is causing a conflict

viktym changed the title ~~Magento Quality Patches - updates~~ Magento Quality Patches - improvement of dev experience Jul 21, 2020

viktym force-pushed the git-based branch 2 times, most recently from 95a0cbb to d2e0b3a Compare July 22, 2020 03:25

mveeramneni added Progress: review PR/issue status Release: 1.0.6 labels Jul 22, 2020

mveeramneni requested a review from shiftedreality July 22, 2020 16:44

oshmyheliuk self-requested a review July 22, 2020 16:44

viktym force-pushed the git-based branch 2 times, most recently from d4140df to 6f52a82 Compare July 22, 2020 20:08

MC-36033: Split ece-patches and magento-patches application

65837e0

viktym force-pushed the git-based branch from 6f52a82 to 65837e0 Compare July 22, 2020 20:18

oshmyheliuk previously approved these changes Jul 22, 2020

View reviewed changes

Merge remote-tracking branch 'magento/develop' into git-based

008df20

mveeramneni requested a review from oshmyheliuk July 23, 2020 17:14

shiftedreality suggested changes Jul 23, 2020

View reviewed changes

viktym dismissed oshmyheliuk’s stale review via 60ce53d July 23, 2020 19:05

viktym force-pushed the git-based branch from 60ce53d to 5a99a21 Compare July 23, 2020 21:26

MC-36033: Split ece-patches and magento-patches application

24cdf11

- fixes after CR

viktym force-pushed the git-based branch from 5a99a21 to 24cdf11 Compare July 23, 2020 21:49

viktym added 2 commits July 24, 2020 10:19

MC-36033: Split ece-patches and magento-patches application

8c2e9f5

- fixes after CR

MC-36033: Split ece-patches and magento-patches application

6ddb1fe

- bump version

shiftedreality previously approved these changes Jul 24, 2020

View reviewed changes

oshmyheliuk previously approved these changes Jul 24, 2020

View reviewed changes

mveeramneni added Progress: qa in progress and removed Progress: review PR/issue status labels Jul 24, 2020

mveeramneni self-assigned this Jul 24, 2020

andriyShevtsov assigned andriyShevtsov and unassigned mveeramneni Jul 24, 2020

MC-36033: Split ece-patches and magento-patches application

1113e58

- fix table render for Symfony < 4

viktym dismissed stale reviews from oshmyheliuk and shiftedreality via 1113e58 July 26, 2020 15:08

mveeramneni requested review from shiftedreality and oshmyheliuk July 27, 2020 15:00

shiftedreality approved these changes Jul 27, 2020

View reviewed changes

oshmyheliuk approved these changes Jul 27, 2020

View reviewed changes

BaDos merged commit 7245166 into magento:develop Jul 27, 2020

bubasuma deleted the git-based branch September 17, 2020 21:35

magento-devops-reposync-svc pushed a commit that referenced this pull request Sep 9, 2022

Merge pull request #61 from magento-commerce/develop

38a601e

MCLOUD-9275: Cloud Tools Release

		} catch (ApplierException $exception) {
		$this->printInfo($output, 'Error: patch conflict happened');

Magento Quality Patches - improvement of dev experience #61

Magento Quality Patches - improvement of dev experience #61

Uh oh!

Conversation

viktym commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Preconditions for test scenarios

Testing scenario 1 - Applying optional quality patch on a local and remote Cloud instance

Testing scenario 2 - test ece-patches 'apply' and 'revert' with QUALITY_PATCHES: '*' on local Cloud instance

Testing scenario 3 - test magento-patches 'apply' and 'revert' on local Cloud instance

Testing scenario 4 - work on Magento git-instance

Release notes

Associated documentation updates

Contribution checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktym Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktym Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andriyShevtsov commented Jul 27, 2020

Uh oh!

Uh oh!

viktym commented Jul 21, 2020 •

edited

Loading

viktym Jul 23, 2020 •

edited

Loading

viktym Jul 23, 2020 •

edited

Loading