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

Rework the packs pack (more packs-related ChatOps!) #2982

Merged
merged 23 commits into from Nov 25, 2016
Merged

Conversation

emedvedev
Copy link
Contributor

@emedvedev emedvedev commented Oct 31, 2016

  1. Aliases for the packs pack:
    pack install for installing a pack or multiple packs (just a minor rework here);
    pack search to query the index;
    pack show to display information about an installed pack, including HEAD location in the git tree (N commits ahead/behind);
    pack show available to fetch information about a pack from the index.

  2. Most of packs.deploy is now obsolete, so I'm removing it and the helper actions for now, but I would appreciate @jjm finding some time to talk it over with me. A thing or two that was possible with packs.deploy might not be covered by our new pack management, and if that's the case, I'm fine with keeping the deploy workflow or breaking it down into multiple actions.

  3. There was a linux pack directory and an action. Inside the packs pack. Apparently, for quite some time, too (I remember @enykeev noticing it). I removed it. Sorry, not sorry.

  4. Some minor cleanups (this PR started as a minor cleanup, actually, but then things just got more and more interesting).

@emedvedev
Copy link
Contributor Author

WIP! Like, really WIP. Don't test it yet, there's still some work to be done.

@emedvedev
Copy link
Contributor Author

emedvedev commented Oct 31, 2016

Btw, pack show available <pack> shows content count if it's present in the index, but pack show <pack> doesn't have content count for installed packs, because we don't have helper functions for content counting yet. @Kami promised to get it done :)

@codecov-io
Copy link

codecov-io commented Oct 31, 2016

Current coverage is 77.11% (diff: 100%)

Merging #2982 into master will increase coverage by 0.22%

@@             master      #2982   diff @@
==========================================
  Files           430        430          
  Lines         21997      21997          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          16914      16962    +48   
+ Misses         5083       5035    -48   
  Partials          0          0          

Powered by Codecov. Last update 02e3c95...4ddbe99

@jjm
Copy link
Member

jjm commented Oct 31, 2016

@emedvedev Yes, I think packs.deploy and it's associated actions can be removed. In fact GitHub pack deployment_event workflow that I added earlier in October does not call packs.deploy and calls packs.install directly.

I think with the release of Stackstorm-exchange the bitbucket rule post_receive_webhook.yaml should be removed.

@emedvedev
Copy link
Contributor Author

emedvedev commented Nov 1, 2016

In fact GitHub pack deployment_event workflow that I added earlier in October does not call packs.deploy and calls packs.install directly.

Once we make the move, that rule and workflow will become a bit simpler, too!

action: packs.install
input:
  packs: [  "<% $.ssh_url %>=<% $.deploy_ref %>" ]

I'll skim through all the packs to make sure all the packs.install and packs.download references are updated when the packs are moved.

@arm4b arm4b self-assigned this Nov 22, 2016
@arm4b arm4b added the refactor label Nov 22, 2016
pack_formatted = None
# Try to get the original yaml file because we know
# how it's stored in the Exchange, otherwise fall back
# to transforming json.
Copy link
Member

@arm4b arm4b Nov 23, 2016

Choose a reason for hiding this comment

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

@emedvedev
As I see, we already getting pack meta from index from endpoint: https://index.stackstorm.org/v1/index.json
And then we're downloading https://index.stackstorm.org/v1/packs/github.yaml again with the same meta.

What was initial idea here? Because it seems a bit dirty with all the hardcoding, additional request, regex.

I'm thinking about removing this second pack.yaml part and rely only on get_pack_from_index(pack) meta.

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'm thinking about removing this second pack.yaml part and rely only on get_pack_from_index(pack) meta.

👍

Problem with json->yaml is that it's not ordered, but that doesn't really matter much.

armab added 3 commits November 24, 2016 18:31
-  `!pack get` - get info about installed pack
-  `!pack show` - show remote StackStorm Exchange pack
-  polished & working `!pack show`
@arm4b arm4b added RFR and removed WIP labels Nov 24, 2016
@arm4b
Copy link
Member

arm4b commented Nov 24, 2016

So it's fixed & Polished & Acceptance tested in Slack.
The new list of ChatOps Action Aliases is adjusted to conform with CLI commands for consistency.

  • !pack install (screenshot) - Install pack(s)
  • !pack get (screenshot) - Get info about locally installed pack
  • !pack search (screenshot) - Search for pack(s) in StackStorm Exchange
  • !pack show (screenshot) - Show pack info from StackStorm Exchange

The Aliases automated tests as well as Integration Tests (e2e, robo) are in my TODO list and will be added, but It's good to merge now to see things in pre-production (packages).

Additionally, I'll need staging-unstable packages to make sure that it works in EL6 (outdated git).

@emedvedev
Copy link
Contributor Author

❤️ ❤️ ❤️ ❤️

LGTM.

@enykeev
Copy link
Member

enykeev commented Nov 25, 2016

enykeev	[10:30] 
!pack show st2

stuffBOT	[10:30]  
@enykeev: Getting back to you about the `st2` pack:
Here's the full index entry at StackStorm Exchange:
{author: st2-dev, description: StackStorm pack management, email: info@stackstorm.com,
    name: st2, repo_url: 'https://github.com/StackStorm-Exchange/stackstorm-st2',
    version: 0.1.2}

To install the pack: `pack install st2`
If the pack is already installed, I will upgrade it to the latest version, keeping the config.

I'm kinda missing hubot alias before the command, though I'm not sure we can get it now in any sane way.

Also, #3051 to prevent different kinds of output formatting for different packs.

Other than that, looks good to me.

@enykeev enykeev changed the title [WIP] Rework the packs pack (more packs-related ChatOps!) Rework the packs pack (more packs-related ChatOps!) Nov 25, 2016
upgrade:
type: "boolean"
default: false
description: "Upgrade the pack if already installed."
Copy link
Member

@enykeev enykeev Nov 25, 2016

Choose a reason for hiding this comment

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

Wait a second. I don't see us using it in any way. I also don't think we should have something like that to begin with. Can settle to default: true, 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.

I'm not sure what that flag is doing here 😛 Should just remove.

Copy link
Member

Choose a reason for hiding this comment

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

Don't know where it comes from too. Will remove.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, but this is interesting.

In future it would be good to have pack install really idempotent.
So if pack is already installed - it won't be reinstalled again, unless some special flag like that specified. Re-downloading/re-installing by default takes time and it's in fact "upgrade" or "reinstall".

This makes sense especially for configuration management tools, when you apply playbook/cookbook, which apart of deploying st2 installs a bunch of st2 packs on the same machine. And it's normal to run same playbook on the same machine several times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bigger discussion that applies to package management in general. It will be idempotent if you specify a precise version, and we already support that, but a repeated install is normally regarded as upgrade, we're certainly not alone here. :)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Installing + Downloading over and over again (even if it reaches the same state) is not considered really "Idempotent" in configuration management.
If resources is in desired state - we shouldn't repeat the same operation again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. They do stay in that state though 😛

Let's make sure we get to that issue after the release, it's pretty important.

@arm4b
Copy link
Member

arm4b commented Nov 25, 2016

I'm kinda missing hubot alias before the command, though I'm not sure we can get it now in any sane way.

Yeah, @enykeev I just verified, and there is no hubot alias variable in context. I bet none could think that it might be useful to have in context. Might be something good to adjust in future.

@arm4b arm4b merged commit 94bc537 into master Nov 25, 2016
@arm4b arm4b deleted the packs-cleanup branch November 25, 2016 16:45
@Kami
Copy link
Member

Kami commented Nov 26, 2016

Nice work everyone!

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.

None yet

7 participants