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

Add argument on app:config:dump to skip dumping all system settings. #12410

Merged

Conversation

jalogut
Copy link
Contributor

@jalogut jalogut commented Nov 22, 2017

Description

Add option config-types on app:config:dump to skip dumping all system settings. That allows to dump only scope, theme for easier maintainability.

Fixed Issues

When using app:config:dump in real projects, we normally do not want to dump all thousands core_config_data settings into config files. That makes the files difficult to read and maintain. Imho dumping scopes and themes makes a lot of sense. However, the system settings should be kept small and simple. They must be progressively added according to project needs.

On the other hand, the main purpose of config.php is to propagate configuration that must be same in all environments, specially for PRD and Build systems. These settings should be just a few, like for example css and js configuration needed for setup:static-content:deploy. Of course others settings might be needed depending on the project but all the others are better managed into core_config_data table or by using setup scripts.

This new option config-types makes possible to dump scopes and themes automatically (needed for build system) while managing system settings manually using config:set --lock-config as requested in PR:

It also fixes this issue:

Manual testing scenarios

Example of workflow on a real project:

  1. When creating the project we dump all settings with app:config:dump scopes themes

  2. That creates the needed settings for scopes and themes but skips system core config data

  3. We also add shared settings needed for PRD and Build environments

    bin/magento config:set --lock-config dev/js/merge_files 1
    bin/magento config:set --lock-config dev/css/merge_css_files 1
    bin/magento config:set --lock-config dev/static/sign 1
    
  4. As project evolves, we add a new stores and themes, so we need to update our config.php settings.

    bin/magento app:config:dump  scopes themes
    
  5. At the same time, if we ever need to share system settings on all environments, we can use the command bin/magento config:set --lock-config without needing to dump all hundreds of settings into the config.php

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@jalogut
Copy link
Contributor Author

jalogut commented Nov 22, 2017

…allows to dump only scope and theme while skipping system for easier maintainability.
@jalogut jalogut force-pushed the config-dump-skip-system-config branch from 7464ffd to 3202e94 Compare November 23, 2017 10:14
@jalogut jalogut changed the title Add option on app:config:dump to skip dumping all system settings. Add argument on app:config:dump to skip dumping all system settings. Nov 23, 2017
@miguelbalparda
Copy link
Contributor

miguelbalparda commented Nov 24, 2017

Thanks for your contribution @jalogut! Can you check the failing tests please? Most of them failed asserting that two strings are equal plus the code sniffer has some suggestions for the new code.
Also, this should probably go to 2.3+ since we are trying to stabilize 2.2 and this seems to be a new feature and not a bugfix.

@miguelbalparda miguelbalparda self-assigned this Nov 24, 2017
@jalogut
Copy link
Contributor Author

jalogut commented Nov 25, 2017

Hi @miguelbalparda Thanks for noticing. I hope it's ok now.

@miguelbalparda
Copy link
Contributor

I'll check internally and report back. In the meantime can you check https://travis-ci.org/magento/magento2/jobs/307090461?

@miguelbalparda miguelbalparda added this to the November 2017 milestone Nov 27, 2017
@jalogut
Copy link
Contributor Author

jalogut commented Nov 28, 2017

@miguelbalparda perfect! I just fixed the issue

@miguelbalparda
Copy link
Contributor

@dmanners what's the best course of action here? Probably we can consider #12441 too and point them to 2.3?

@dmanners
Copy link
Contributor

Hey @jalogut and @miguelbalparda the rule of thumb at the moment is new features to 2.3

@miguelbalparda
Copy link
Contributor

@jalogut can you edit the PR and point it to 2.3 instead of 2.2? We can consider backports later too.

@jalogut
Copy link
Contributor Author

jalogut commented Dec 1, 2017

Hi @miguelbalparda

#12441 was just merged into 2.2. Imho this feature addresses exactly same issues that make not possible to use the new deployment pipeline added in 2.2. If the other PR was merged, it would make sense to merge that one too.

@okorshenko okorshenko added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 11, 2017
@okorshenko okorshenko closed this Dec 11, 2017
@okorshenko okorshenko reopened this Dec 11, 2017
@okorshenko okorshenko removed the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 11, 2017
@jalogut
Copy link
Contributor Author

jalogut commented Mar 31, 2018

Hi, any updates on this PR? Do you think this feature will be merged anytime soon?

@enriquei4
Copy link
Contributor

Hi @miguelbalparda @dmanners , as @jalogut ask. Could we expect this feature in 2.2?

@miguelbalparda
Copy link
Contributor

This is probably not going to be included in the next release, which is 2.3 at this point. I'll check internally and have this one reassigned. Thanks for the wait!

@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018
@sidolov sidolov self-assigned this Apr 16, 2018
@magento-engcom-team
Copy link
Contributor

@jalogut thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@sidolov
Copy link
Contributor

sidolov commented Apr 17, 2018

Hi @jalogut , I'm processing this PR now, it would be great if you create additional PR to devdocs repository with information about new features.

magento-team pushed a commit that referenced this pull request Apr 22, 2018
@magento-engcom-team magento-engcom-team merged commit 6751504 into magento:2.2-develop Apr 22, 2018
@magento-engcom-team
Copy link
Contributor

Hi @jalogut. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

@jalogut
Copy link
Contributor Author

jalogut commented Jun 28, 2018

@magento-engcom-team it seems this PR was not included in 2.2.5 as mentioned on your last comment. Do you know what happened?

@dmanners
Copy link
Contributor

Sorry @jalogut this was a problem with the bot configuration. It was using the wrong date for the cut-off point for pull requests getting into the 2.2.5 release. This PR should make it into the 2.2.6 release instead.

Sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants