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

Removal of st2mistral from basic chapter of docs #1011

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

amanda11
Copy link
Contributor

@amanda11 amanda11 commented Aug 14, 2020

Removal of st2mistral from the Basic chapter.

Addresses part of the st2docs section of StackStorm/st2#4762

@amanda11 amanda11 marked this pull request as draft August 14, 2020 14:55
@amanda11
Copy link
Contributor Author

Couple of points:

  1. Chatops and notify and orquesta - was no section for this before, but the parameter was mentioned in the orquesta pages but no notify example. I have taken the example from the st2/st2tests/st2tests/fixtures/packs/orquesta_tests/actions/notify.yaml but couldn't get to this to trigger notify events
  2. I suggest this is not merged until after the PRs on install, references and troubleshooting as they all reference the mistral. This PR could be included, as it has taken a copy of files in the other PRs.

@winem
Copy link
Contributor

winem commented Aug 14, 2020

Looks good considering @amanda11 s recent comment.

@amanda11 amanda11 marked this pull request as ready for review August 17, 2020 12:23
name: <% $.name %>
ip: <% $.ip %>

The sections below contain additional YAQL examples of how to work with lists and dictionaries,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a lot of useful general info here on YAQL. In the Orquesta pages the approach seems to have been to just point to the YAQL and Jinja projects themselves?

Are we happy to remove the pages and just refer to the YAQL/Jinja projects or want to put a page together for Orquesta/YAQL and Orquesta/Jinja?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would vote for a page for Orquesta/YAQL and Orquesta/Jinja. But this shouldn't be a blocker for this PR imho.

required: true
type: string
default: Stanley
notify:
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 the example used in the unit-tests, but I couldn't get notify in Orquesta working in my setup - but I may be missing something. In the Orquesta pages we just mention there is a notify parameter that we can ignore for now...

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 got Orquesta notifications working but I also had to add on a notify section as well as the notify parameters, e.g. I also had:

notify:
  on-complete:
    routes:
      - slack
    message: "Succeeded"

so that it had the details of route etc.

Do we think the documentation as is is ok, or

  1. should I add in the notify section into the example
  2. Should I add in a preceding sentence along following lines:

The method for specify the route and conditions for when tasks in an Orquesta workflow is the same as ActionChain. You have a notify section in the action meta when registering.

and should I put in a notify section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LindsayHill @armab Having got notifications working in Orquesta now, I'm wondering if more needs to go into this
section on the chatops page. Or alternatively I was mis-understanding something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LindsayHill @armab Having got notifications working in Orquesta now, I'm wondering if more needs to go into this
section on the chatops page. Or alternatively I was mis-understanding something.

So as not to confuse things, I will merge this PR. And generate a new PR with enhancements that I think will make Orquesta notifications even clearer.

Copy link
Member

Choose a reason for hiding this comment

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

👍 step by step

@arm4b arm4b requested a review from a team August 17, 2020 13:36
@arm4b arm4b added this to the 3.3.0 milestone Aug 17, 2020
Copy link
Contributor

@LindsayHill LindsayHill left a comment

Choose a reason for hiding this comment

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

Thanks for this work. Made a few small changes. Looks good.

@arm4b
Copy link
Member

arm4b commented Aug 17, 2020

@amanda11 now as you have Github repo permissions, as PR author feel free to merge it when you see the fit.

@amanda11 amanda11 merged commit 4e065c3 into StackStorm:master Aug 18, 2020
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.

4 participants