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

Add Load Impact Automatic Performance Testing #75

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

populist
Copy link
Contributor

This is integration with Load Impact's V2 REST API. Once you figure a test in Load Impact and provide your API Key, Quicksilver will kick off a test and give you a URL at which you can review the results.

// For extra security, you can store this information in
// the private area of the files directory as documented
// at https://github.com/pantheon-systems/quicksilver-examples.
$api_key = 'add-api-key-here';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@populist Should this switch to using secrets.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had left the API key defined in the variable since I think it is simpler to get running and the API key is stored in a way that isn't web accessible.

Obviously using secrets.json is going to be even better and I added a documentation link to show people how to do this if they want.

Do you (or @greg-1-anderson or @joshkoenig) think we should standardize our examples using secrets.json?

Copy link
Member

@greg-1-anderson greg-1-anderson Apr 21, 2016

Choose a reason for hiding this comment

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

The advantage of using secrets.json is that you can then install this example via terminus quicksilver install, and set your secrets via terminus secrets set key value, with no need to alter the code.

The code as written has the advantage of being short, and not requiring any duplicated functions to manage secrets. So, right now there is a fair bit of duplicate code in the Quicksilver examples regarding secrets. There is a discussion on that topic in #74.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should standardize on an abstraction layer. Part of my reasoning is for the security benefit. While some of our quicksilver examples involve keys that aren't terribly sensitive I think we should just stick with the best practice of "don't commit keys to any repo" so as limit confusion.

Just as important for me is clearing a path toward treating QS scripts as modules/plugins as opposed to custom code. Right now these scripts are examples meant to be edited by the implementor. There should not be an expectation (right now) that a Slack notification script in one Pantheon site is identical to a Slack notification script on another Pantheon site.

I anticipate that expectation will evolve. Our user base of Drupal and WordPress developers is accustomed to running commands like drush dl modulename and treating community code as not-to-be-edit (hacking core kills kittens and all that). With terminus quicksilver install examplename there is a road to treating QS code like modules to which configuration is passed.

Of course Quicksilver as a whole is still evolving. And maybe for the stage we are at now we should be actively fighting (possibly) premature abstraction. If we want Quicksilver users to think "this is code I need to read, understand and edit to my use case" then let's continue to include pieces like this that require editing the php file. If we want QS users to think "I install an example and it just works" then we need abstractions like key management.

@joshkoenig
Copy link
Member

@stevector Thanks for your comments. The vision is right on, but:

Of course Quicksilver as a whole is still evolving. And maybe for the stage we are at now we should be actively fighting (possibly) premature abstraction.

That's where we are at. Right now we need people to understand that they "own" their QS implementation. Either editing the code, or actively managing a secrets.json file accomplish this goal.

Going beyond that will likely mean taking things out of the Quicksilver Slace, e.g. native integration of external repos, slack, etc. Some of this is on the roadmap, and some of it is not, but the value should become clear as we see what people are doing more and more.

Secrets management is the one sticky wicket. We don't have a product answer on the roadmap, and demanding people use the key module may be setting too high a bar. We should discuss this more.

@stevector
Copy link
Collaborator

Thinking about this more, I'm less enthusiastic about evolving toward "Quicksilver Plugin" as a first class concept. QS can call Drush/WP-CLI and it can call Drupal/WordPress directly. Each already has their own plugin/module system. For use cases where DRY/abstraction is of high value (like the 100 sites question on today's couch coding) we can recommend encapsulating code in one of these more mature systems. And I think saying "use the Slack notifier contrib module/plugin" is better for us than building and maintaining Pantheon-native Slack notification.

Secrets management is tough, but at least it is not a QS-specific problem. Plenty of Drupal/WordPress sites just keep keys for external services as plain text in the DB, which carries a similar set of risks as committing them to a repo. This reminds me of the "product vs. practice" conversation we had a few months back when I was in the SF office. We don't necessarily need a Pantheon key management product as long as we point the way toward the generic best practice.

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.

5 participants