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

Added new parameters #14

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Mte90
Copy link

commented Mar 22, 2019

Require to test it, fix Varying-Vagrant-Vagrants/VVV#1632:

    boilerplate:
        repo: git@github.com:Varying-Vagrant-Vagrants/custom-site-template.git
        custom:
            delete_default_plugins: true
            plugins: 
                - query-monitor
                - https://github.com/crstauf/query-monitor-extend/archive/version/1.0.zip
                - https://github.com/norcross/airplane-mode/archive/master.zip
            locale: it_IT
        nginx_upstream: php72
        hosts:
            - boilerplate.test

@Mte90 Mte90 added the enhancement label Mar 23, 2019

@Mte90 Mte90 self-assigned this Mar 23, 2019

@Mte90 Mte90 requested a review from tomjn Mar 23, 2019

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 23, 2019

And finished :-D
Now instal new plugins, remote the default, also set the locale and add new constants as true in wp-config.php

@tomjn

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@Mte90 you need to install a package in your editor to show tabs and spaces differently, your YAML test case is broken ( mixed tabs and spaces )

@tomjn

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Also the config section might be misleading, I can foresee somebody trying to use it to specify arbitrary values for wp-config.php, getting confused, and opening an issue, how can we solve that?

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 25, 2019

I saw later that my editor not support Yaml -.-'

For config maybe we can change it like `constants' because after all adding one there means that s added as true and is more clear.

@tomjn

This comment has been minimized.

Copy link
Member

commented on d49a3d6 Mar 25, 2019

I don't think this resolves the problem, e.g. what if one tries to use this to specify parameters that aren't equal to true? I don't believe that the YAML format works here, else we might get people raising support issues for trying to do things like this:

    constants:
        - SITE_ID_CURRENT_SITE: 2

Which won't work, my initial thoughts are that this is not a simple thing and it should be removed until a better solution is devised:

  • the yaml doesn't work for anything other than setting it as true
  • what if the user changes their mind and wants it set to false
  • if we have arbitrary values, how do we differentiate between 2 and "2"?
  • what happens if they try to specify the database name, then use this to specify the database name again?
  • Would these not be better being 2 level up as full parameters?

for example:

custom:
    wp_debug: true

And do we really want WP_DEBUG_LOG? I'd prefer we have WP_DEBUG always on, then log to a dedicated file for that site at Nginx level if we can figure that out.

It might also be better to support a tool such as Daniel Bachubers dictator

This comment has been minimized.

Copy link
Author

replied Mar 25, 2019

I was thinking this for the kind of constants used on wp-config.php useful from the start of creating a new website (because it use the wp core on installing).
So if think that for this reason is enough, if we want that for other reasons the yaml probably is not the best option.

This comment has been minimized.

Copy link
Member

replied Mar 25, 2019

I understand that, but we have to be mindful of how it could be taken because it impacts the UX and at the end of the day we're the ones answering the support requests :) I say move it out of this PR so we can look at merging the other parts of it

This comment has been minimized.

Copy link
Author

replied Mar 25, 2019

Ok I will split to another pr that part :-)

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

Removed the config support.
In the meantime I will study better yaml and how we can do that for custom value of the constants.

Show resolved Hide resolved provision/vvv-init.sh Outdated
Show resolved Hide resolved provision/vvv-init.sh Outdated
Show resolved Hide resolved provision/vvv-init.sh
Show resolved Hide resolved README.md Outdated

Mte90 added some commits Apr 24, 2019

@Mte90

This comment has been minimized.

Copy link
Author

commented Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.