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

Allow adding Chassis as a Composer dependancy #583

Open
wants to merge 30 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kasparsd

kasparsd commented Sep 22, 2018

Fixes #481.

Objectives

Allow adding Chassis to any project as a Composer or npm dependancy:

composer require --dev chassis/chassis

with only two extra files:

Vagrantfile that loads the Chassis Vagrantfile and specifies the project root as the directory that contains the configuration file:

ENV['CHASSIS_CWD'] = File.dirname(__FILE__)
load "vendor/chassis/chassis/Vagrantfile"

and config.local.yaml that adds the project directory as a plugin or theme to Chassis:

hosts:
  - composer.local

synced_folders:
  .: /vagrant/content/plugins/this-plugin

Suggested Changes

  • Always map Chassis root directory to the /vagrant directory inside the Vagrant machine.

  • Map the Vagrant working directory (directory of Vagrantfile) or ENV['CHASSIS_CWD'] (see discussion) to /chassis inside Vagrant.

  • Always look for config override files config.local.yaml in /chassis.

By default, the Vagrant working directory is the same as the Chassis root directory so /chassis will map to to the same place as /vagrant. However, in case of adding Chassis via Composer or npm, the /chassis directory will map to the root of that project while /vagrant will still map to the root of the Chassis core.

Settings for paths in config.local.yaml are still resolved relative to the Chassis root directory, in order to preserve the default lookups for the wp and contents directories.

Tasks

  • All Chassis system files should be loaded relative to the Chassis root directory.
  • Allow config.yaml overrides in a directory outside the Chassis root.

Examples

Example: Plugin Development

See https://github.com/wpsh/chassis-composer-demo/tree/dev-481

  • Example of a plugin with an included development environment via Composer.
  • Installs the Query Monitor plugin for development purposes.
  • Uses synced_folders to define the project root directory as a plugin mapped to /chassis/content/plugins/this-plugin.
git clone https://github.com/wpsh/chassis-composer-demo.git
cd chassis-composer-demo
git checkout dev-481
composer install
vagrant up

Example: WP Core Development

See https://github.com/wpsh/chassis-composer-demo/tree/dev-481-wp-develop

Known Issues

  • None.

Notes

See: https://kaspars.net/blog/wordpress/wordpress-environment-composer

@BronsonQuick

This comment has been minimized.

Show comment
Hide comment
@BronsonQuick

BronsonQuick Sep 22, 2018

Member

Thanks so much for starting work on this @kasparsd! Let me know when you're happy for one of us to test it out for you.

Member

BronsonQuick commented Sep 22, 2018

Thanks so much for starting work on this @kasparsd! Let me know when you're happy for one of us to test it out for you.

@BronsonQuick BronsonQuick changed the title from Allow adding Chassis as a Composer dependancy to WIP Allow adding Chassis as a Composer dependancy Sep 22, 2018

Show outdated Hide outdated Vagrantfile Outdated
Show outdated Hide outdated Vagrantfile Outdated
# Check that submodules have been loaded
if not File.exist?(File.join(File.dirname(__FILE__), "puppet", "modules", "apt", ".git"))
if not File.exist?(File.join(chassis_dir, "puppet", "modules", "apt", ".git"))

This comment has been minimized.

@kasparsd

kasparsd Sep 24, 2018

File.dirname(__FILE__) was repeated in several places so we introduce chassis_dir.

@kasparsd

kasparsd Sep 24, 2018

File.dirname(__FILE__) was repeated in several places so we introduce chassis_dir.

kasparsd added a commit to wpsh/chassis-composer-demo that referenced this pull request Sep 24, 2018

@kasparsd kasparsd changed the title from WIP Allow adding Chassis as a Composer dependancy to Allow adding Chassis as a Composer dependancy Sep 24, 2018

@kasparsd

This comment has been minimized.

Show comment
Hide comment
@kasparsd

kasparsd Sep 24, 2018

@BronsonQuick @rmccue This is ready for code review and testing. Please see the "Demo" section in the description of the pull request for how to test this.

kasparsd commented Sep 24, 2018

@BronsonQuick @rmccue This is ready for code review and testing. Please see the "Demo" section in the description of the pull request for how to test this.

kasparsd added some commits Sep 24, 2018

Account for @@dir being equal to @@config_dir
Use relative path for the Chassis root sync definition

@kasparsd kasparsd changed the title from Allow adding Chassis as a Composer dependancy to WIP Allow adding Chassis as a Composer dependancy Sep 24, 2018

@kasparsd

This comment has been minimized.

Show comment
Hide comment
@kasparsd

kasparsd Oct 6, 2018

@BronsonQuick This is ready for code review and testing. See the Examples section in the description for two sample repositories.

kasparsd commented Oct 6, 2018

@BronsonQuick This is ready for code review and testing. See the Examples section in the description for two sample repositories.

@kasparsd kasparsd changed the title from WIP Allow adding Chassis as a Composer dependancy to Allow adding Chassis as a Composer dependancy Oct 6, 2018

@BronsonQuick BronsonQuick requested a review from rmccue Oct 6, 2018

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Oct 8, 2018

Contributor

@kasparsd I think you've potentially mixed two different directories here? Extensions/Puppet/etc should always be at /vagrant/extensions, /vagrant/puppet, etc, I don't think we want to change that. (We could potentially change to always mount root at /chassis though.)

Perhaps I'm missing what this gains over the existing VAGRANT_CWD?

Contributor

rmccue commented Oct 8, 2018

@kasparsd I think you've potentially mixed two different directories here? Extensions/Puppet/etc should always be at /vagrant/extensions, /vagrant/puppet, etc, I don't think we want to change that. (We could potentially change to always mount root at /chassis though.)

Perhaps I'm missing what this gains over the existing VAGRANT_CWD?

@kasparsd

This comment has been minimized.

Show comment
Hide comment
@kasparsd

kasparsd Oct 11, 2018

@rmccue Thanks for reviewing the pull request!

Extensions/Puppet/etc should always be at /vagrant/extensions, /vagrant/puppet, etc.

Just to confirm -- could we mount Chassis root at /vagrant and the project root at /chassis? I'll look into this.

Perhaps I'm missing what this gains over the existing VAGRANT_CWD?

I created this sample project to illustrate the issue.

With VAGRANT_CWD=vendor/chassis/chassis vagrant up it looks for config overrides config.local.yaml inside the Chassis root directory vendor/chassis/chassis instead of the project directory. This is due to this lookup logic which assumes that all config overrides are at the Chassis root @@dir.

The suggested fix is to introduce a @@config_dir variable which can be set to anything via the CHASSIS_CWD environmental variable. We need some way of telling Chassis that it should read the config override from another directory.

kasparsd commented Oct 11, 2018

@rmccue Thanks for reviewing the pull request!

Extensions/Puppet/etc should always be at /vagrant/extensions, /vagrant/puppet, etc.

Just to confirm -- could we mount Chassis root at /vagrant and the project root at /chassis? I'll look into this.

Perhaps I'm missing what this gains over the existing VAGRANT_CWD?

I created this sample project to illustrate the issue.

With VAGRANT_CWD=vendor/chassis/chassis vagrant up it looks for config overrides config.local.yaml inside the Chassis root directory vendor/chassis/chassis instead of the project directory. This is due to this lookup logic which assumes that all config overrides are at the Chassis root @@dir.

The suggested fix is to introduce a @@config_dir variable which can be set to anything via the CHASSIS_CWD environmental variable. We need some way of telling Chassis that it should read the config override from another directory.

.DS_Store
.idea
config.local.yaml
/.wp-cli
/wp-cli.local.yml
/extensions

This comment has been minimized.

@kasparsd

kasparsd Oct 11, 2018

This directory is actually being tracked as it contains the example extension.

@kasparsd

kasparsd Oct 11, 2018

This directory is actually being tracked as it contains the example extension.

This comment has been minimized.

@rmccue

rmccue Oct 11, 2018

Contributor

This is intentional; gitignore simply ensures untracked files are not shown, but still allows files to be committed within it. Chassis requires the example extension for technical reasons (plus it's useful), but we don't want to show any other files in that directory.

@rmccue

rmccue Oct 11, 2018

Contributor

This is intentional; gitignore simply ensures untracked files are not shown, but still allows files to be committed within it. Chassis requires the example extension for technical reasons (plus it's useful), but we don't want to show any other files in that directory.

This comment has been minimized.

@kasparsd

kasparsd Oct 11, 2018

@rmccue Right, so we want to keep tracking /extensions directory and the example directory inside it while ignoring everything else. This is why I suggest we introduce a .gitignore in the extensions directorythat only tracks theexample` directory and excludes everything else.

Currently it is a bit confusing to see the extensions directory ignored in the .gitignore but actually tracked in the repository. I think it only works because it was once tracked and later ignored in 9b49b56. Per docs:

Files already tracked by Git are not affected.

and

To stop tracking a file that is currently tracked, use git rm --cached.

So if the intention is to track the sample extension extensions/sample, I suggest we clarify that by updating the .gitignore files.

I'm totally OK with not changing this, if you prefer. Just wanted to explain the reasoning and though behind it.

@kasparsd

kasparsd Oct 11, 2018

@rmccue Right, so we want to keep tracking /extensions directory and the example directory inside it while ignoring everything else. This is why I suggest we introduce a .gitignore in the extensions directorythat only tracks theexample` directory and excludes everything else.

Currently it is a bit confusing to see the extensions directory ignored in the .gitignore but actually tracked in the repository. I think it only works because it was once tracked and later ignored in 9b49b56. Per docs:

Files already tracked by Git are not affected.

and

To stop tracking a file that is currently tracked, use git rm --cached.

So if the intention is to track the sample extension extensions/sample, I suggest we clarify that by updating the .gitignore files.

I'm totally OK with not changing this, if you prefer. Just wanted to explain the reasoning and though behind it.

Show outdated Hide outdated .gitignore Outdated
@@ -3,6 +3,7 @@
module Chassis
@@dir = File.dirname(File.dirname(__FILE__))
@@config_dir = @@dir

This comment has been minimized.

@kasparsd

kasparsd Oct 11, 2018

This is the new config path variable used to reference the directory that contains the config overrides. Previously it was assumed that all config overrides are inside the Chassis root directory.

@kasparsd

kasparsd Oct 11, 2018

This is the new config path variable used to reference the directory that contains the config overrides. Previously it was assumed that all config overrides are inside the Chassis root directory.

.DS_Store
.idea
config.local.yaml
/.wp-cli
/wp-cli.local.yml
/extensions

This comment has been minimized.

@rmccue

rmccue Oct 11, 2018

Contributor

This is intentional; gitignore simply ensures untracked files are not shown, but still allows files to be committed within it. Chassis requires the example extension for technical reasons (plus it's useful), but we don't want to show any other files in that directory.

@rmccue

rmccue Oct 11, 2018

Contributor

This is intentional; gitignore simply ensures untracked files are not shown, but still allows files to be committed within it. Chassis requires the example extension for technical reasons (plus it's useful), but we don't want to show any other files in that directory.

Show outdated Hide outdated .gitignore Outdated
end
# Set up the paths as needed
config["mapped_paths"] = {}
config["mapped_paths"]["base"] = "/vagrant"

This comment has been minimized.

@rmccue

rmccue Oct 11, 2018

Contributor

This still appears to be using the incorrect split of base/Chassis directories?

@rmccue

rmccue Oct 11, 2018

Contributor

This still appears to be using the incorrect split of base/Chassis directories?

This comment has been minimized.

@kasparsd

kasparsd Oct 11, 2018

Yeah, this is a hard one.

Currently we set the base relative to the Chassis root directory @@dir and without a base override in config.local.yaml it defaults to the Chassis root directory which is where it looks for the default wp and content directories. And per you suggestion, the core Chassis directory should be kept at /vagrant which also includes the default wp and content directories.

By default, if base is inside the Chassis root, it would map the base to /vagrant which contains the default wp and content directories provided by Chassis.

If the base in config.local.yaml is set to outside the Chassis root if not base.start_with? @@dir, it would map that base to /chassis and do the wp and content mapping relative to that /chassis directory. And with a custom base, the user is expected to configure paths to the wp and content directories.

With the Composer/npm use-case the project root directory will always be outside the Chassis directory so we need to default to the wp and contents directories provided by Chassis (under /vagrant) unless specified otherwise via the paths override.

Mapping the base to /vagrant satisfies both requirements because:

  • it defaults to the wp and content directories provided by Chassis even if the project root is outside the Chassis root and no paths overrides have been specified;

  • it looks for custom wp and content directories (relative to the Chassis root directory) only if a custom base is specified in config.local.yaml.

Finally, we now map the project root to /chassis so syncing anything to /chassis/#{path} will create new wp and content directories inside the project root.

@kasparsd

kasparsd Oct 11, 2018

Yeah, this is a hard one.

Currently we set the base relative to the Chassis root directory @@dir and without a base override in config.local.yaml it defaults to the Chassis root directory which is where it looks for the default wp and content directories. And per you suggestion, the core Chassis directory should be kept at /vagrant which also includes the default wp and content directories.

By default, if base is inside the Chassis root, it would map the base to /vagrant which contains the default wp and content directories provided by Chassis.

If the base in config.local.yaml is set to outside the Chassis root if not base.start_with? @@dir, it would map that base to /chassis and do the wp and content mapping relative to that /chassis directory. And with a custom base, the user is expected to configure paths to the wp and content directories.

With the Composer/npm use-case the project root directory will always be outside the Chassis directory so we need to default to the wp and contents directories provided by Chassis (under /vagrant) unless specified otherwise via the paths override.

Mapping the base to /vagrant satisfies both requirements because:

  • it defaults to the wp and content directories provided by Chassis even if the project root is outside the Chassis root and no paths overrides have been specified;

  • it looks for custom wp and content directories (relative to the Chassis root directory) only if a custom base is specified in config.local.yaml.

Finally, we now map the project root to /chassis so syncing anything to /chassis/#{path} will create new wp and content directories inside the project root.

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Oct 11, 2018

Contributor

I wonder whether we could try and address both this issue and #581 at the same time.

What if instead of a custom environment variable for this, we instead loaded config{,.local}.yaml from the specified content directory? This would require a two-pass parse, but would be a bit nicer IMO.

Contributor

rmccue commented Oct 11, 2018

I wonder whether we could try and address both this issue and #581 at the same time.

What if instead of a custom environment variable for this, we instead loaded config{,.local}.yaml from the specified content directory? This would require a two-pass parse, but would be a bit nicer IMO.

puts "WARNING: Submodules may be missing, and could not automatically\ndownload them for you."
end
# Extra new line, please!
puts
end
# Ensure we have a content directory to sync.
chassis_content_dir = File.join(chassis_dir, "content")
Dir.mkdir(chassis_content_dir) unless File.exists?(chassis_content_dir)

This comment has been minimized.

@kasparsd

kasparsd Oct 11, 2018

@rmccue We need the content directory to exist before Vagrant can sync it to the virtual machine or it fails with the following error on the initial vagrant up:

There are errors in the configuration of this machine. Please fix
the following errors and try again:

vm:
* The host path of the shared folder is missing: /absolute/path/to/Chassis/content
@kasparsd

kasparsd Oct 11, 2018

@rmccue We need the content directory to exist before Vagrant can sync it to the virtual machine or it fails with the following error on the initial vagrant up:

There are errors in the configuration of this machine. Please fix
the following errors and try again:

vm:
* The host path of the shared folder is missing: /absolute/path/to/Chassis/content
@kasparsd

This comment has been minimized.

Show comment
Hide comment
@kasparsd

kasparsd Oct 11, 2018

What if instead of a custom environment variable for this, we instead loaded config{,.local}.yaml from the specified content directory?

@rmccue This still assumes that you're running vagrant from the Chassis root directory where it finds the config.local.yaml. #583 is more supporting config.local.yaml outside the Chassis root directory.

I would like to better understand the example provided by @mikeselander:

 - /chassis (Everything Chassis)
   - config.local.yaml (Basic overrides (see full file below)
 - /content (Actual used content directory)
   - config.local.yaml (Installation-specific overrides)
 - /wordpress (WordPress used on platform)

What is the reason behind having two config.local.yaml files? How are they different?

kasparsd commented Oct 11, 2018

What if instead of a custom environment variable for this, we instead loaded config{,.local}.yaml from the specified content directory?

@rmccue This still assumes that you're running vagrant from the Chassis root directory where it finds the config.local.yaml. #583 is more supporting config.local.yaml outside the Chassis root directory.

I would like to better understand the example provided by @mikeselander:

 - /chassis (Everything Chassis)
   - config.local.yaml (Basic overrides (see full file below)
 - /content (Actual used content directory)
   - config.local.yaml (Installation-specific overrides)
 - /wordpress (WordPress used on platform)

What is the reason behind having two config.local.yaml files? How are they different?

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