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

Chatops integration #55

Merged
merged 35 commits into from
Apr 16, 2019
Merged

Chatops integration #55

merged 35 commits into from
Apr 16, 2019

Conversation

rapittdev
Copy link
Contributor

@rapittdev rapittdev commented Mar 15, 2019

Closes #17

Proposal for chatops integration base on #17

As mentioned in the tracking issue:

  • Update st2docs
  • start with defining a Dockerfile for st2chatops
  • st2chatops configuration could be controlled by passing ENV variables to override defaults
  • creating a K8s resource for st2chatops as well as corresponding Helm values

find out more about the why here

@ghost
Copy link

ghost commented Mar 15, 2019

Thank you @armab for the suggestions. I ended up doing that after tested the other approaches. as suggested, This is a base for discussions/suggestions.

I took this repository as a stepping stone for the new integration, There aren't much of diversion from it; However, there might be unnecessary values passed around which is left for cleaning up.

Corresponding st2-dockerfile PR In my approach I am using st2chatops package. TBD if there's a better source.

I probably need a few more days to bring it to a clean state, feedback, contributions very welcome as I won't be able to commit time next week.

If there's a better/alternative approach, feel free to close this PR.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Can you please sync-up your branch with the upstream master, so we'll see only the diff from this specific change?

@ghost
Copy link

ghost commented Mar 19, 2019 via email

Copy link
Member

@arm4b arm4b 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 the contribution! 👍

I provided some first early comments that'll help to course correct and move forward future st2chatops support in ST2 K8s/Helm.

values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
chatops-settings.yaml Outdated Show resolved Hide resolved
values.yaml Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

At a high level, I see it as the following way to configure chatops via Helm values.yaml

[....]

# StackStorm ChatOps (https://docs.stackstorm.com/chatops/index.html)
# As hubot can't be HA scaled properly, we deploy only single replica of st2chatops
st2chatops:
  # Enable st2chatops (default: false)
  enabled: true
  # Custom hubot adapter ENV variables to pass through which will override st2chatops.env defaults.
  # See https://github.com/StackStorm/st2chatops/blob/master/st2chatops.env
  # for the full list of supported adapters and example ENV variables.
  env:
    HUBOT_ADAPTER: slack
    HUBOT_SLACK_TOKEN: xoxb-CHANGE-ME-PLEASE
  # Use custom generated st2chatops Docker image 
  image:
  #  repository: stackstorm
  #  name: st2chatops
  #  tag: {{ .Chart.AppVersion }}
  #  pullPolicy: Always
  resources: {}
  # Additional advanced settings to control pod/deployment placement
  nodeSelector: {}
  tolerations: []
affinity: {}

Meaning we'll pass additional/custom ENV variables to st2chatops container that will be applied on top of defaults. This brings flexibility of configuring any existing or future added new adapters like StackStorm/st2chatops#121

@arm4b
Copy link
Member

arm4b commented Mar 25, 2019

Looks like the fork/branch still needs to be synced with upstream properly as Github diff shows unrelated to this PR changes https://github.com/StackStorm/stackstorm-ha/pull/55/files

@ghost ghost mentioned this pull request Mar 26, 2019
@rapittdev
Copy link
Contributor Author

Since I'm try

Looks like the fork/branch still needs to be synced with upstream properly as Github diff shows unrelated to this PR changes https://github.com/StackStorm/stackstorm-ha/pull/55/files

Might it happen because I have local branch set to something other than master ?
I tried...

git pull upstream 

git remote -v shows

origin	https://github.com/rapittdev/stackstorm-ha.git (fetch)
origin	https://github.com/rapittdev/stackstorm-ha.git (push)
upstream	https://github.com/StackStorm/stackstorm-ha.git (fetch)
upstream	https://github.com/StackStorm/stackstorm-ha.git (push)

@ghost
Copy link

ghost commented Mar 27, 2019

@armab I think we're better off closing this PR when all the details are cleared and submit a new one with proper implementation. If you agree I'll get started with that. This was just an early setup to better get a grasp of what we're dealing with. I have to force push my changes in order to be able to sync with upstream again. Sorry about that.

rapittdev and others added 2 commits April 5, 2019 09:14
@rapittdev
Copy link
Contributor Author

rapittdev commented Apr 5, 2019

@armab @warrenvw I tried syncing with upstream and after some-time wrestling, I think this is best I got. I am not sure if that's updated since I see merging blocks. But I have an idea. If you agree you can open a new branch for chops so the community can directly contribute to. I think that'll save us time.

I also was able to deploy to k8 while pulling the image of the st2-docker file. I added some cleanups on my code which I will cherry peak into the new branch.

** Sorry about any type while mobile, I'll fix it later**

@arm4b
Copy link
Member

arm4b commented Apr 5, 2019

@rapittdev As I see, the github diff still shows old content that was already contributed to upstream master branch. We can't open an all-writeable branch for community per Github limitations.
But if you want to open a new PR instead of this one, - that would be fine too.

Copy link
Contributor Author

@rapittdev rapittdev left a comment

Choose a reason for hiding this comment

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

Thank you. With help from the StackStrom community, we were able to deploy to k8. This is still a wimp since we're waiting for feedback on how to proceed.

Chart.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
chatops-settings.yaml Outdated Show resolved Hide resolved
templates/configmaps_chatops.yaml Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
@arm4b arm4b added RFR and removed WIP labels Apr 12, 2019
@arm4b arm4b changed the title [WIP] Chatops integration Chatops integration Apr 12, 2019
@arm4b arm4b requested review from warrenvw and removed request for warrenvw April 12, 2019 19:53
@rapittdev
Copy link
Contributor Author

👍👍👍 Thanks @armab @warrenvw This chart was already ahead, now it's about to get wings! Thank you for helping us finalize this. We're planning to launch our product with Stackstorm-Ha!

@arm4b
Copy link
Member

arm4b commented Apr 12, 2019

@mosn Thank you for starting this K8s Chatops work, making changes and bringing this feature to a working state! Appreciate your contribution 👍

Just added a little bit of polishing here and there and tried to find possible corner cases. One of them is reported in StackStorm/st2chatops#124
@warrenvw I'll wait for your additional review in this PR and @mosn if you can double-check 👀 and verify this PR in action too, - that would help before we merge it early next week.

@ghost
Copy link

ghost commented Apr 13, 2019

@armab I'll be able to start the integration at 15:00 CET. I will provide updates as I progress through the implementation.

@ghost
Copy link

ghost commented Apr 13, 2019

Cluster is ready based on following best practice

https://github.com/bitnami/kube-prod-runtime/blob/master/docs/quickstart-gke.md

Going to start with Stackstorm and chatops now

Copy link
Contributor

@warrenvw warrenvw left a comment

Choose a reason for hiding this comment

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

LGTM! Well done. 👍

The code is easy to understand.

@ghost
Copy link

ghost commented Apr 16, 2019

it works great too!

image

@armab I had to move to EWC 3.0 because of this. So far the improved workflow was able to do 5 hours job in 30min. Very helpful with a syntax error. As for chatops I'll finish converting all my mistral chains and then look at that, hopefully, I can work out detailed feedback later in this week.

Just quick info, the description for this chart is an accurate description of my experience.

@arm4b arm4b merged commit 968816b into StackStorm:master Apr 16, 2019
@arm4b arm4b deleted the chatops-ha branch April 16, 2019 16:14
arm4b pushed a commit to StackStorm/st2docs that referenced this pull request Apr 16, 2019
@cognifloyd cognifloyd removed the RFR label Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add st2chatops
5 participants