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.

Sign up for GitHub

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

Jump to bottom

[FEAT-219] Remove the ability to set drush version, use pantheon.yml instead #1190

Merged
merged 2 commits into from
Sep 13, 2016
Merged

[FEAT-219] Remove the ability to set drush version, use pantheon.yml instead #1190

merged 2 commits into from
Sep 13, 2016

Conversation

marktheunissen
Copy link
Contributor

marktheunissen
marktheunissen reviewed Sep 13, 2016
View reviewed changes
php/Terminus/Models/Environment.php
* @param array $settings Key/value pairs to set in the environment settings
* @return bool
*/
private function updateSetting(array $settings = []) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TeslaDethray This function doesn't seem to be used anymore in this codebase - I would remove it, but I'm not sure if that impacts plugins or has any other undesirable effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're moving away from directly setting these variables altogether. If any are still in use then they probably need to be moved to panthon.yml. I don't know if any plugins are relying on this function though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we definitely are

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're moving away from it and the core does not use it, I feel confident that we can remove this function. Thanks for being so thorough!

@ronan
Copy link
Contributor

ronan commented Sep 13, 2016

Probably needs a note in the CHANGELOG

@marktheunissen marktheunissen changed the title Remove the ability to set drush version, use pantheon.yml instead [FEAT-219] Remove the ability to set drush version, use pantheon.yml instead Sep 13, 2016
@TeslaDethray
Copy link
Contributor

CI is failing because the feature test was still present. I've removed it. Once it passes CI, +1.

@ronan
Copy link
Contributor

ronan commented Sep 13, 2016

👍

@coveralls
Copy link

coveralls commented Sep 13, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 14.05% when pulling c861da4 on remove_set_drush_version into d57ad1e on master.

@TeslaDethray TeslaDethray merged commit 0f26f81 into master Sep 13, 2016
@TeslaDethray TeslaDethray deleted the remove_set_drush_version branch September 13, 2016 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Ready to Ship
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marktheunissen @ronan @TeslaDethray @coveralls