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

minor updates to the mattermost connector page #4634

Merged
merged 10 commits into from Oct 31, 2019
Merged

minor updates to the mattermost connector page #4634

merged 10 commits into from Oct 31, 2019

Conversation

btotharye
Copy link
Contributor

Proposed changes:

  • Made a few updates to the Mattermost connector doc page to clarify a few things more.

Status (please check what you already did):

  • made PR ready for code review
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

@btotharye btotharye requested a review from akelad October 18, 2019 07:25
@btotharye btotharye added the type:docs 📖 Improvements to the documenation. Adding missing pieces or improving existing ones. label Oct 18, 2019
@btotharye
Copy link
Contributor Author

made a few updates to the mattermost docs if you want to let me know any feedback @akelad

tmbo
tmbo reviewed Oct 18, 2019
View reviewed changes
docs/user-guide/connectors/mattermost.rst Outdated
@@ -44,6 +44,9 @@ run script, e.g. using:

you need to supply a ``credentials.yml`` with the following content:

**Remember the user value is the actual username of your bot user, not displayname**
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather put this into the yaml

  mattermost:
     url: "https://chat.example.com/api/v4"
     team: "community"
     user: "user@user.com"  #  actual username of your bot user, not displayname
     pw: "password"
     webhook_url: "https://server.example.com/webhooks/mattermost/webhook"

Copy link
Member

Choose a reason for hiding this comment

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

@btotharye what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea agreed its a better way I'll update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @tmbo this should be updated now let me know what you think

@tmbo tmbo changed the base branch from master to 1.4.x October 21, 2019 15:21
@tmbo tmbo removed the request for review from akelad October 21, 2019 15:23
tmbo
tmbo requested changes Oct 21, 2019
View reviewed changes
docs/user-guide/connectors/mattermost.rst Outdated
@@ -44,6 +44,9 @@ run script, e.g. using:

you need to supply a ``credentials.yml`` with the following content:

**Remember the user value is the actual username of your bot user, not displayname**
Copy link
Member

Choose a reason for hiding this comment

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

@btotharye what do you think?

@tmbo
Copy link
Member

tmbo commented Oct 22, 2019

diff doesn't look right. you can't pull in master, this is a PR against the release branch 1.4.x (so that we can release these documentation changes as part of the next patch release instead of waiting and sitting around for the next minor).

I'd suggest to revert the last two commits and redo the change without pulling in master.

@btotharye
Copy link
Contributor Author

gotcha sorry I'll fix it was in the middle of a few PR's and got confused

@btotharye
Copy link
Contributor Author

btotharye commented Oct 25, 2019 •
edited

hey @tmbo just to make sure I'm fixing this right the last commit I see now in my git log is the following:

commit 00273f4c917b0106fdcd4c18fc8433764d14ab54 (master)
Merge: 976e1ab917 988a0d20aa
Author: Tom Bocklisch <tom@rasa.com>
Date:   Tue Oct 22 11:17:28 2019 +0200

    merged 1.4.x

This should be the right one to now do my changes on again right? The only reason I ask is it says now my remote is behind but not sure which origin branch to pull from. I'm guessing I need to remove those remote commits I did somehow maybe to resolve this issue here.

@tmbo
Copy link
Member

tmbo commented Oct 25, 2019

yes that sounds good 👍

@btotharye
Copy link
Contributor Author

ok @tmbo I think we are good I believe I put the update in the right section of the changelog as well let me know if I didn't.

Thanks

@btotharye
Copy link
Contributor Author

well minus the changelog conflict I'll have to look into fixing that

@btotharye
Copy link
Contributor Author

ok @tmbo this should be all cleaned up now, let me know any further updates

@btotharye
Copy link
Contributor Author

went ahead and updated from the latest 1.4.x updates

@btotharye btotharye requested a review from tmbo October 29, 2019 18:59
tmbo
tmbo approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

looks perfect now 👍

@tmbo tmbo merged commit 681c889 into 1.4.x Oct 31, 2019
@tmbo tmbo deleted the update_mattermost_docs branch October 31, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmbo tmbo tmbo approved these changes

Assignees
No one assigned
Labels
type:docs 📖 Improvements to the documenation. Adding missing pieces or improving existing ones.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@btotharye @tmbo