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

427: System configuration full and updates export. #15

Merged

Conversation

owlsmage
Copy link
Contributor

@owlsmage owlsmage commented Nov 18, 2020

Module to observe and export configuration changes to message queue.

Description (*)

  • Allows full export manually or automatically (recurring upgrade data script)
  • Observe changed values in system configuration to export only updates
  • Exports values for affected stores (only store view level)

Related Pull Requests

https://github.com/magento-commerce/magento2-infrastructure/pull/1395v

Fixed Issues (if relevant)

  1. Fixes Configuration propagation catalog-storefront#427

Code Review Checklist (*)

  • Story AC is completed
  • proposed changes correspond to Magento Technical Vision
  • new or changed code is covered with web-api/integration tests (if applicable)
  • no backward incompatible changes

@mslabko
Copy link
Member

mslabko commented Nov 18, 2020

Looks good to me!

Regarding AC
"collected configuration paths should be described in di.xml"
I think we may need some Filter class, which will accept only the system path, described in di.xml. This class should filter paths during "observer" and "full sync" part.
We have to filter it to prevent potential security issues and simplify security audit in the future

Ideally need to have possibility to accept paths from env variables (can be done be adding "init_paramter" const)
<argument name="exportConfigPaths" xsi:type="init_parameter">some\corresponding\path::CONST_NAME</argument>) or create new system configuration which can be dynamically configured per each instance

$whitelistedPaths = $this->whitelistProviderPool->getWhitelist();

foreach ($configData as $item) {
if (in_array($item['path'], $whitelistedPaths)) {
Copy link
Member

Choose a reason for hiding this comment

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

can we add the ability to add configuration with partial match, like "path/"?
E.g. to provide all paths for layered navigation from app/code/Magento/LayeredNavigation/etc/config.xml we can use "catalog/layered_navigation/
" path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mslabko partial match added

*/
private function publishMessage(array $configData, string $evenType): void
{
$message = $this->messageBuilder->build($evenType, $configData);
Copy link
Member

Choose a reason for hiding this comment

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

need to add check if message present, since we can receive empty results after filtration by whitelist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mslabko added check to do not publish empty message to queue

*/
class FullSyncCommand extends \Symfony\Component\Console\Command\Command
{
const COMMAND_NAME = 'config-export:sync:full';
Copy link
Member

Choose a reason for hiding this comment

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

lets rename command to "commerce-data-export::config:export"

{
if (empty($this->whitelist)) {
try {
$this->whitelist = $this->deploymentConfig->get(self::WHITELIST_CONFIG_KEY, []);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add CLI command which will add configuration to env.php ? `bin/magento commerce-data-export::config:add-paths-to-whitelist"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mslabko command added.

Avoid publishing message to queue if message is empty after filtration by whitelist.
@owlsmage owlsmage requested a review from mslabko November 22, 2020 16:51
@owlsmage
Copy link
Contributor Author

@mslabko i added clean up queue before test run.

@RuslanKostiv1
Copy link
Contributor

@owlsmage please add https://github.com/magento-commerce/magento2-infrastructure/pull/1395 in the list of Related Pull Requests

@RuslanKostiv1
Copy link
Contributor

@magento import pr to magento-commerce/commerce-data-export

@magento-engcom-team
Copy link

@RuslanKostiv1 the pull request successfully imported.

@mmansoor-magento mmansoor-magento merged commit 933b513 into magento:main Dec 16, 2020
magento-devops-reposync-svc pushed a commit that referenced this pull request Nov 4, 2021
MDEE-57: Handle is_deleted updates. Unassign by chunks
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.

Configuration propagation
6 participants