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

Implement a new kubectl transport #60

Merged

Conversation

rudolfbyker
Copy link
Contributor

@rudolfbyker rudolfbyker commented Jan 28, 2022

Overview

This pull request:

Q A
Bug fix? no
New feature? yes
Has tests? yes
BC breaks? no
Deprecations? no

Summary

Implement a new kubectl transport for accessing Drupal instances running in Kubernetes.

Description

If you have this alias:

foo:
  paths:
    drush-script: /vv/vendor/bin/drush
  root: /vv/web
  uri: drupal.example.com
  kubectl:
    tty: false
    interactive: false
    namespace: vv
    resource: deploy/drupal-deployment
    container: drupal

... you can do this:

drush @foo status

... or any other drush command, and it will run via kubectl, like this:

kubectl --namespace=vv exec --tty=false --stdin=false deploy/drupal-deployment --container=drupal -- /vv/vendor/bin/drush status --uri=drupal.example.com --root=/vv/web

@rudolfbyker rudolfbyker marked this pull request as ready for review January 28, 2022 08:48
@rudolfbyker
Copy link
Contributor Author

@greg-1-anderson Do you have any comments on how the yaml structure under kubectl in the site alias should be designed? It's currently super simple:

foo:
  paths:
    drush-script: /vv/vendor/bin/drush
  root: /vv/web
  uri: drupal.example.com
  kubectl:
    tty: false
    interactive: false
    namespace: vv
    resource: deploy/drupal-deployment
    container: drupal

I'm wondering whether we would ever put anything other than transport info under kubectl. But I doubt it.

*/
public function wrap($args)
{
# TODO: How/where do we complain if a required argument is not available?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-1-anderson How/where do we complain if a required argument in is not available in the site alias? Is there a "standard" way (e.g. a specific custom exception that should be thrown) in this repo?

Copy link
Member

Choose a reason for hiding this comment

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

Throwing an exception would be best; however, at the moment, this library does not provide any custom exception types. You could provide one, if you wanted, or maybe just use InvalidArgumentException. Maybe that's just similar enough to be confusing, though. Recommendations welcome.

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 would leave the TODO there, merge it, and make a new issue to address the need for custom exceptions. It needs to be designed well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#62

Copy link
Member

@greg-1-anderson greg-1-anderson left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just decide what you want to do about parameter validation & I'll merge.

@rudolfbyker
Copy link
Contributor Author

Did you see my comment above? Ready to merge?

@greg-1-anderson
Copy link
Member

Sure, let's go ahead and put this in. Thanks.

@greg-1-anderson greg-1-anderson merged commit 8f371ef into consolidation:main Feb 19, 2022
@rudolfbyker rudolfbyker deleted the dev-kubectl-transport branch March 1, 2022 09:35
@rudolfbyker
Copy link
Contributor Author

rudolfbyker commented Mar 1, 2022

Thanks for the merge. I see this is working well for SSH, but not for rsync commands. Any idea why/why not? Do rsync commands use a different mechanism? I don't find anything related to rsync in this repo, other than strings in the tests.

I'm trying to achieve something like this transparently: https://serverfault.com/a/887402/301389

@weitzman
Copy link
Member

Ideally we document this in README

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants