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

Bundle install via registry #988

Merged
merged 7 commits into from
Sep 28, 2016
Merged

Bundle install via registry #988

merged 7 commits into from
Sep 28, 2016

Conversation

vanstee
Copy link
Member

@vanstee vanstee commented Sep 26, 2016 •
edited by christophermaier
Loading

This adds support for installing a bundle via the registry.

Example:

@vanstee: !bundle install heroku
@cog: Bundle "heroku" version "0.0.4" installed.

@vanstee: !bundle install pagerduty 0.0.1
@cog: Bundle "pagerduty" version "0.0.1" installed.

I'll follow this up with a bundle uninstall subcommand as well.

@vanstee vanstee added the review label Sep 26, 2016
christophermaier
christophermaier requested changes Sep 27, 2016
View reviewed changes
lib/cog/bundle_registry.ex
%HTTPotion.Response{status_code: 200, body: body} ->
{:ok, Poison.decode!(body)}
%HTTPotion.Response{status_code: 404} ->
{:error, "Bundle not found"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably have a catch-all case for other errors, too (e.g., registry is unavailable, registry crashes, etc.) just so we don't crash Cog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

lib/cog/commands/bundle.ex
@@ -19,14 +19,10 @@ defmodule Cog.Commands.Bundle do
"info <bundle>" => "Detailed information on an installed bundle",
"enable <bundle> [<version>]" => "Enable a specific version of an installed bundle",
"disable <bundle>" => "Disable an installed bundle",
"versions <bundle>" => "List all installed versions for a given bundle"
"versions <bundle>" => "List all installed versions for a given bundle",
"install <bundle>[:<version>]" => "Install latest or specified version of bundle from registry"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we went with version being an optional standalone argument, instead of tacked on with a colon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Updated the usage but missed this. Thanks.

lib/cog/commands/bundle.ex
Installation and uninstallation of bundles cannot currently be done via
chat; please use `cogctl` for this functionality.
"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay! 🎊

test/cog/chat/slack/templates/embedded/bundle_install_test.exs

expected = """
Bundle "heroku" version "0.0.4" installed.
""" |> String.strip
Copy link
Collaborator

Choose a reason for hiding this comment

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

For short-and-sweet templates like this, a single line is fine, instead of the whole triple-quote-and-strip dance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah just some rushed copy-pasta. Fixing.

test/integration/commands/bundle_test.exs
@@ -61,4 +61,8 @@ defmodule Integration.Commands.BundleTest do
assert_error_message_contains(response, "Unknown subcommand 'not-a-subcommand'")
end

test "installing a bundle", %{user: user} do
[payload] = send_message(user, "@bot: operable:bundle install heroku 0.0.4")
assert %{name: "heroku", version: "0.0.4"} = payload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Musing out loud here... it may be better for composability to have bundle-related commands return the same shape of data.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think they do. This just uses Bundle's implementation of Poison.encode IIRC. Should I be rendering a view instead?

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 try to use the views when we can... that way we have one central place to do the JSON conversion, and the Cog commands look just like the API requests. Documentation for one can double as documentation for the other.

And if /when we ever get around to calling embedded commands via the API, it'll be purely a backend change, without any user-visible effects.

lib/cog/commands/bundle.ex
Installation and uninstallation of bundles cannot currently be done via
chat; please use `cogctl` for this functionality.
"""

permission "manage_commands"

rule "when command is #{Cog.Util.Misc.embedded_bundle}:bundle must have #{Cog.Util.Misc.embedded_bundle}:manage_commands"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat tangential to this PR, but we should probably consider adding some allow rules for the list, info, and versions subcommands.

(Locking down installation of bundles, of course, is definitely the way to go, though 😄 )

priv/templates/embedded/bundle-install.greenbar
@@ -0,0 +1 @@
Bundle "~$results[0].name~" version "~$results[0].version~" installed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should wrap this in an ~each var=$results~ tag to handle cases where people install a bunch of bundles at once:

for bundle in heroku pingdom twitter | bundle install $bundle

Otherwise, we'll just show the installation of the first bundle.

(Alternatively, you could do a table for the more-than-one bundle scenario; that might be overkill in this case, though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will do.

@kevsmith
Copy link
Member

Two questions:

  1. Do we have an issue tracking this work so we don't miss it in the changelog?
  2. What's the plan for installing bundles via cogctl?

@vanstee
Copy link
Member Author

vanstee commented Sep 27, 2016

Answers:

  1. Nope but I'll make one.
  2. There's an open PR, but it needs some work before merging. I think @christophermaier is right that we need an API endpoint in Cog to install bundles from the registry.

@vanstee vanstee force-pushed the vanstee/bundle-install-via-registry branch from 0f54e20 to d4bfce4 Compare September 27, 2016 20:34
@vanstee vanstee merged commit a4a71f5 into master Sep 28, 2016
@vanstee vanstee deleted the vanstee/bundle-install-via-registry branch September 28, 2016 14:20
@vanstee vanstee removed the review label Sep 28, 2016
@vanstee vanstee added this to the Cog 0.15 milestone Sep 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophermaier christophermaier christophermaier requested changes

Assignees

@vanstee vanstee

Labels
None yet
Projects
None yet
Milestone
Cog 0.15
Development

Successfully merging this pull request may close these issues.
3 participants
@vanstee @kevsmith @christophermaier