Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

magento/devdocs#: Configure migration. Changes. #5184

Merged
merged 4 commits into from
Aug 27, 2019

Conversation

atwixfirster
Copy link
Contributor

@atwixfirster atwixfirster commented Aug 14, 2019

Purpose of this pull request

Regarding to Magento best practices we should not edit core files except when you contribute 😸

Configure migration contains recommendation to directly edit Data Migration Tool config files. This pull request replaces mentioned recommendations regarding to Magento best practices.

Affected DevDocs pages

whatsnew
Added best practices for configuring the migration tool.

@devops-devdocs
Copy link
Collaborator

An admin must run tests on this PR before it can be merged.

Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @atwixfirster. Thank you for this way of thinking. First of all, your advice is very useful. However, we don't have a convincing explanation of why should we overcomplicate the process and create a separate module. Let me shed some light on it.

First of all, by copying *.xml.dist files to *.xml we don't edit the core, we just add new files. The same happens when you copy phpunit.xml.dist to phpunit.xml in case of configuring the automated tests.
In this particular case, I would say, the problem is in bringing manual changes to the vendor directory. Ideally, this directory should be modified only by composer. It's not a sort of "good" practice but MUST in case if you use continuous delivery for your environments.
If you use continuous delivery, you install the codebase from scratch on every deployment (the vendor directory is generated by composer completely). That's why all your changes in vendor/magento/data-migration-tool/ will be lost.
There are two obvious ways of solving this issue:

  • a separate module in app/code directory with proper configs under version control.
  • a separate module installable via composer with the proper configs (in this case you need an extra repository or modified composer install flow).

Providing changes in vendor/magento/data-migration-tool is acceptable only in case if you run the migration process locally and if you are not going to put your changes under version control.

@rogyar rogyar added 2.2.x 2.3.x Magento 2.3 related changes Major Update Significant original updates to existing content Technical Updates to the code or processes that alter the technical content of the doc labels Aug 14, 2019
@atwixfirster atwixfirster force-pushed the migration-tool-configure branch from 36bdbb6 to d343b7d Compare August 15, 2019 05:33
@atwixfirster
Copy link
Contributor Author

Hi, @rogyar !

Thank you VERY MUCH for the explanation.

I've implemented your suggestions. Could you please verify them and let me know what do you think?

Have a wonderful day!

@atwixfirster atwixfirster force-pushed the migration-tool-configure branch from d343b7d to ca3898b Compare August 16, 2019 04:35
@rogyar
Copy link
Contributor

rogyar commented Aug 27, 2019

Awesome, thank you

@jeff-matthews jeff-matthews removed the Technical Updates to the code or processes that alter the technical content of the doc label Aug 27, 2019
@atwixfirster
Copy link
Contributor Author

@jeff-matthews, all your suggestions have been implemented. Please review.

Many thanks as always :)

Copy link
Contributor

@jeff-matthews jeff-matthews left a comment

Choose a reason for hiding this comment

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

Almost there @atwixfirster ;). Just a few more edits please.

@jeff-matthews
Copy link
Contributor

@atwixfirster, there are also some markdown syntax errors:

rake test:md
Testing Markdown style with mdl ...
guides/v2.2/migration/migration-tool-configure.md:70: MD009 Trailing spaces
guides/v2.3/migration/migration-tool-configure.md:70: MD009 Trailing spaces

A detailed description of the rules is available at https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md

The Markdown linter has found 4 issues

@atwixfirster
Copy link
Contributor Author

@jeff-matthews, let's try again :)

Thanks

@jeff-matthews
Copy link
Contributor

Confirmed! Thanks @atwixfirster.

@jeff-matthews
Copy link
Contributor

running tests

@jeff-matthews jeff-matthews merged commit acae6e8 into magento:master Aug 27, 2019
@ghost
Copy link

ghost commented Aug 27, 2019

Hi @atwixfirster, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2.2.x 2.3.x Magento 2.3 related changes Major Update Significant original updates to existing content Partner: Atwix partners-contribution PR created by Magento partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants