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

Use config.storage.sync to instantiate the FileStorage #2471

Merged
merged 1 commit into from Jan 23, 2017

Conversation

Projects
None yet
5 participants
@webflo
Contributor

webflo commented Nov 30, 2016

This should make config_split compatible cex/cim.

@weitzman

This comment has been minimized.

Show comment
Hide comment
@weitzman

weitzman Nov 30, 2016

Member

LGTM. We had a spurious test fail that cleared up on re-test.

I can merge this once you think it is complete. I notice that the title says WIP.

Member

weitzman commented Nov 30, 2016

LGTM. We had a spurious test fail that cleared up on re-test.

I can merge this once you think it is complete. I notice that the title says WIP.

@webflo

This comment has been minimized.

Show comment
Hide comment
@webflo

webflo Dec 1, 2016

Contributor

Lets get some feedback from config_split maintainers first.

Contributor

webflo commented Dec 1, 2016

Lets get some feedback from config_split maintainers first.

@weitzman

This comment has been minimized.

Show comment
Hide comment
@weitzman

weitzman Dec 12, 2016

Member

Any feedback yet?

Member

weitzman commented Dec 12, 2016

Any feedback yet?

@swentel

This comment has been minimized.

Show comment
Hide comment
@swentel

swentel Dec 12, 2016

Not a maintainer, but supporter and user. Patch makes very much sense, so +1!

swentel commented Dec 12, 2016

Not a maintainer, but supporter and user. Patch makes very much sense, so +1!

@weitzman

This comment has been minimized.

Show comment
Hide comment
@weitzman

weitzman Jan 5, 2017

Member

FYI I added back storage filters hook in #2314 for better integration with config_split. Not sure if this PR is still desirable/needed.

Member

weitzman commented Jan 5, 2017

FYI I added back storage filters hook in #2314 for better integration with config_split. Not sure if this PR is still desirable/needed.

@bircher

This comment has been minimized.

Show comment
Hide comment
@bircher

bircher Jan 23, 2017

Contributor

I am a maintainer of config_split and I love this PR. In fact this would be a far superior approach to storage filters.
I am moving to an approach that I swap out the config.storage.sync service and return the wrapped storage that unites the main directory and the split directories of the active splits. If drush would use that storage I could even think about retiring the custom commands for importing and exporting.
The storage filters - while inspired by the storage filter drush uses - would mean a much more complex interaction, I just recently had to change the interface a bit to fix a bug and that would require a change to drush etc. In addition, if drush and drupal console and drupal core all use the same service all work as expected.

The only thing that might need extra work would be the extra functionality drush cex and cim have such as automatically committing to git etc. But in my opinion that is not really blocking.

Contributor

bircher commented Jan 23, 2017

I am a maintainer of config_split and I love this PR. In fact this would be a far superior approach to storage filters.
I am moving to an approach that I swap out the config.storage.sync service and return the wrapped storage that unites the main directory and the split directories of the active splits. If drush would use that storage I could even think about retiring the custom commands for importing and exporting.
The storage filters - while inspired by the storage filter drush uses - would mean a much more complex interaction, I just recently had to change the interface a bit to fix a bug and that would require a change to drush etc. In addition, if drush and drupal console and drupal core all use the same service all work as expected.

The only thing that might need extra work would be the extra functionality drush cex and cim have such as automatically committing to git etc. But in my opinion that is not really blocking.

@webflo webflo changed the title from WIP: Use config.storage.sync to instantiate the FileStorage to Use config.storage.sync to instantiate the FileStorage Jan 23, 2017

@webflo

This comment has been minimized.

Show comment
Hide comment
@webflo

webflo Jan 23, 2017

Contributor

Alright, i think we have an agreement. LGTM :)

Contributor

webflo commented Jan 23, 2017

Alright, i think we have an agreement. LGTM :)

@weitzman weitzman merged commit edca15b into drush-ops:8.x Jan 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@weitzman

This comment has been minimized.

Show comment
Hide comment
@weitzman

weitzman Jan 23, 2017

Member

Merged into master and 8.x. I think we can get rid of Drush's filter and our filter hook and just expect folks to take over the sync service. What do folks think?

Member

weitzman commented Jan 23, 2017

Merged into master and 8.x. I think we can get rid of Drush's filter and our filter hook and just expect folks to take over the sync service. What do folks think?

@webflo webflo deleted the webflo:config-sync branch Jan 23, 2017

AdamPS added a commit to AdamPS/drush that referenced this pull request Feb 1, 2017

@bkosborne

This comment has been minimized.

Show comment
Hide comment
@bkosborne

bkosborne Feb 24, 2017

Now that Drush 8.1.10 has been released and includes this code, what needs to be done on my end to get drush cex/cim aware of config split?

bkosborne commented Feb 24, 2017

Now that Drush 8.1.10 has been released and includes this code, what needs to be done on my end to get drush cex/cim aware of config split?

@weitzman

This comment has been minimized.

Show comment
Hide comment
@weitzman

weitzman Feb 24, 2017

Member

Config split folks are working on that in https://www.drupal.org/node/2855319 and similar issues. They would better know the answer to this question.

Member

weitzman commented Feb 24, 2017

Config split folks are working on that in https://www.drupal.org/node/2855319 and similar issues. They would better know the answer to this question.

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