Skip to content

Conversation

thrijith
Copy link
Member

Fixes #5327

  • Creates a global config path in default path if one doesn't exist already.

@thrijith thrijith requested a review from a team as a code owner June 18, 2020 16:20
@thrijith
Copy link
Member Author

@schlessera The change is to handle the case when the default config doesn't exist and isn't set by WP_CLI_CONFIG_PATH.

The change creates a file by default if it doesn't exist at home directory, should the function get_global_config_path accept a parameter to handle creation?

@schlessera
Copy link
Member

The change creates a file by default if it doesn't exist at home directory, should the function get_global_config_path accept a parameter to handle creation?

You mean to skip creating a default one? I don't think that would be needed. Worst case if you want to avoid populating the user home folders with these files, you could always point WP_CLI_CONFIG_PATH to an existing empty file or something.

Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

Just thinking now that we should actually only create the file when we try to write something in there.

Otherwise, we're cloaking the fact that there is no configuration yet.

@thrijith
Copy link
Member Author

thrijith commented Jul 5, 2020

@schlessera The change is to handle the case when the default config doesn't exist and isn't set by WP_CLI_CONFIG_PATH.

The change creates a file by default if it doesn't exist at home directory, should the function get_global_config_path accept a parameter to handle creation?

Just thinking now that we should actually only create the file when we try to write something in there.
Otherwise, we're cloaking the fact that there is no configuration yet.

@schlessera That is what I thought initially so that it doesn't affect other commands flow create a file only when asked so we can add param in this function and when someone uses _alias add or update create one?

@schlessera
Copy link
Member

Yes, I think that's better. Commands can individually request whether the file should be created on-demand or not.

@thrijith
Copy link
Member Author

thrijith commented Jul 5, 2020

@schlessera Updated the code to create file only when asked for, not sure why the test is failing due to that https://travis-ci.org/github/wp-cli/wp-cli/jobs/705166173#L645-L660

thrijith and others added 2 commits July 6, 2020 01:08
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
@schlessera
Copy link
Member

I think the failing test is because:

  • We don't use WPCLI_CONFIG_PATH to point the global config to a path that is controlled (and thus reset) by Behat. Instead, it uses a path that is shared by all tests: Using default global config: /tmp/wp-cli-home/.wp-cli/config.yml (0.029s)
  • The aliases.features runs before these, so that creates a global config in this shared location.

@schlessera
Copy link
Member

I think this might be something we should fix in wp-cli/wp-cli-tests, redirecting the global config location into a folder that is separate per run.

@schlessera
Copy link
Member

Oh, as it turns out, we're already using the global config here: https://github.com/wp-cli/wp-cli-tests/blob/dfcc8b22b51d97150ee66e7655fb618b8bd460b5/features/bootstrap/FeatureContext.php#L66-L94

Then I'm not sure why that test even succeeded in the first place, though... ?

@schlessera
Copy link
Member

Trying an easier solution for now: removing existing config files in this particular test before each run.

@schlessera
Copy link
Member

Hmm, the $HOME variable is not being replaced, unfortunately. I'll have to adapt wp-cli/wp-cli-tests either way.

@schlessera schlessera added this to the 2.5.0 milestone Jul 5, 2020
@schlessera schlessera merged commit cb7a8af into master Jul 5, 2020
@schlessera schlessera deleted the fix/create-global-config branch July 5, 2020 21:52
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.

Error when creating an alias.
2 participants