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 groups to pack metadata #3214

Closed
wants to merge 2 commits into from
Closed

Add groups to pack metadata #3214

wants to merge 2 commits into from

Conversation

emedvedev
Copy link
Contributor

@emedvedev emedvedev commented Feb 11, 2017

This feature adds a groups field to pack.yaml for action grouping (currently used in Automation Suites).

See https://github.com/emedvedev/stackstorm-suite for an example.

@codecov-io
Copy link

codecov-io commented Feb 11, 2017

Codecov Report

Merging #3214 into master will decrease coverage by -0.03%.

@@            Coverage Diff            @@
##           master   #3214      +/-   ##
=========================================
- Coverage   77.83%   77.8%   -0.03%     
=========================================
  Files         433     433              
  Lines       22398   22400       +2     
=========================================
- Hits        17433   17428       -5     
- Misses       4965    4972       +7
Impacted Files Coverage Δ
st2common/st2common/models/api/pack.py 98.18% <100%> (+0.02%)
st2common/st2common/models/db/pack.py 100% <100%> (ø)
st2common/st2common/bootstrap/sensorsregistrar.py 80.58% <ø> (-4.85%)
st2api/st2api/controllers/v1/packs.py 89.51% <ø> (-3.7%)
st2api/st2api/controllers/v1/actionalias.py 79.63% <ø> (-2.78%)
st2common/st2common/query/base.py 83.46% <ø> (-2.26%)
st2api/st2api/controllers/v1/actionexecutions.py 87.83% <ø> (-1.74%)
st2common/st2common/util/action_db.py 94.57% <ø> (-1.55%)
st2common/st2common/bootstrap/triggersregistrar.py 83.33% <ø> (-1.19%)
st2common/st2common/transport/consumers.py 86.32% <ø> (-1.05%)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce0eb95...6223af7. Read the comment docs.

@enykeev
Copy link
Member

enykeev commented Feb 13, 2017

To me, this change is no different to adding new API. And with that, I need to know how else this groups can be used outside of Suites context and have a better scheme describing this groups.

@Kami
Copy link
Member

Kami commented Feb 13, 2017

Yeah, overall I'm fine with a high-level concept (adding a new "generic" attribute for that), but need to think more about the name (perhaps tags instead of the groups or similar).

In addition to that, it would also be great to do that @enykeev - another API schema for the groups item. I did look at the example, but I found "suites" attribute a big confusing - https://github.com/emedvedev/stackstorm-suite/blob/master/pack.yaml

Also to confirm, right now it looks like actions can only be referenced from the same pack, right? I thought the idea was to also allow referencing actions from other packs (suites also being cross-pack grouping mechanism).

In any case, I'm fine with that, but just some additional clarification would be nice :)

@emedvedev
Copy link
Contributor Author

@Kami see https://github.com/StackStorm/bwc-ui/pull/5#issuecomment-279305082 for an illustration of how groups would be used in BWC.

The problem was described in an earlier discussion: BWC packs contain high-level workflows that are going to be used most of the time, and low-level actions that aren't going to be used outside of workflows. Users want a way to filter out the low-level actions and also group the workflows by purpose, hence the groups.

suites is there because one pack can, according to Ram, be present in several suites, so a group can be related to one or more. It's also shown in Web UI (see the screenshot), and acts as a filtering criteria (if I filter by "ipfabric", I will see all the groups that have ipfabric in the suite list.

Groups only contain pack actions, there's no cross-pack grouping (I asked Ram specifically, having the same concern).

@enykeev There's no clear use outside of BWC. I agree that it's changing API, but right now I'd consider it reserved for future use.

@enykeev
Copy link
Member

enykeev commented Feb 13, 2017

I'm not ok with phrasing "reserved for future use" when no one has any idea on how it can be used in future outside of the use case we've specifically agreed not to make API changes for. I realize that you want to get over with it as quickly as possible, but I'm not going to +1 something I was against the entire time.

@emedvedev
Copy link
Contributor Author

@enykeev basically, you are right: we don't have any idea about how it can be used in the future. :) That said, I do not have a good way to implement this feature without API changes, but I'm 100% open to suggestions.

@enykeev
Copy link
Member

enykeev commented Feb 13, 2017

@emedvedev my proposal was to keep this data inside Suite UI and do filtering and grouping client side. In this particular case you would have to have a json dict in suites app matching action names with suites. Suites developers would then have to open a PR there to update this dict when suite composition changes. Eventually, I'm hoping they would take over this Suite UI and keep core team out of the loop.

@lakshmi-kannan
Copy link
Contributor

This is not how we should do this. The data modeling here doesn't seem right to me. I am with enykeev on this. This is a premature change - one that we can't back away easily from. Until the data model solidifies while we are playing around with the UI and get more information from product owners, let's do what @enykeev says. Keep this logic away from st2. Note that this is also used by open source people. Suites for them has no meaning for them whatsoever.

@emedvedev
Copy link
Contributor Author

👍
Closing.

@emedvedev emedvedev closed this Feb 14, 2017
@LindsayHill LindsayHill deleted the feature/pack-groups branch October 3, 2018 19:00
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.

None yet

5 participants