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 messages option #125

Merged

Conversation

martindekov
Copy link
Sponsor Contributor

@martindekov martindekov commented Jun 29, 2019

Adding messages option to the derek bot in order
to apply tempaltes or boilerplate things that we write
all the time.

Signed-off-by: Martin Dekov (VMware) mdekov@vmware.com

Description

Extending the comments handler, by passing the derek config file (.DEREK.yml), in order to take the messages field from it and pass it to the executing function which sends the message to the Issue.

Motivation and Context

  • I have raised an issue to propose this change (required)

Closes #109

How Has This Been Tested?

Manually multi line literal value of the message:
image

Note in order for this to work, the string literal should be placed just below the | like so:

message:
  - name: slack
    value: |
            --
            Join Slack to connect with the community
            https://docs.openfaas.com

If we go like this, this will lead to parsing error

message:
  - name: slack
    value: |
         --
         Join Slack to connect with the community
         https://docs.openfaas.com

Manually single line value of message:
image

Whole content of .DEREK.yml:
image

Image is here martindekov/derek:0.0.7, in order to test augment your .DEREK.yml file like pointed in the picture above and swap the images.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes. 50%
  • All new and existing tests passed.

@alexellis
Copy link
Owner

In the YAML files perhaps message should be messages?

Could we have an alias of Derek msg, what do you think? I use GitHub a lot on mobile, less typing is better for me.

@martindekov
Copy link
Sponsor Contributor Author

Yes Alex your proposals are great I will add those.

@martindekov
Copy link
Sponsor Contributor Author

Change added and tested:

image
Response:

image

When trying to add message whic is not applied, response reads:

image

@martindekov
Copy link
Sponsor Contributor Author

martindekov commented Jun 30, 2019

Also the config:

image

@alexellis
Copy link
Owner

/set reviewer: rgee0

@derek derek bot requested a review from rgee0 July 2, 2019 20:52
Copy link
Contributor

@rgee0 rgee0 left a comment

Choose a reason for hiding this comment

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

I'm only part way through. Will pick up again later

handler/commentHandler.go Show resolved Hide resolved
handler/commentHandler.go Outdated Show resolved Hide resolved
handler/commentHandler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rgee0 rgee0 left a comment

Choose a reason for hiding this comment

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

Hello Martin. Finally got around to finishing up the review. For the time being I've put aside that I don't particularly like message as a command label.

USER_GUIDE.md Outdated Show resolved Hide resolved
handler/commentHandler_test.go Outdated Show resolved Hide resolved
handler/commentHandler.go Outdated Show resolved Hide resolved
handler/commentHandler_test.go Outdated Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
@martindekov
Copy link
Sponsor Contributor Author

Thank you @rgee0 I will refer your comments tomorrow. I am open to suggestions on naming the feature. I went with message as it was mostly discussed in the Issue.

@alexellis
Copy link
Owner

I think:

Feature toggle and definition within YAML: custom_messages or just messages
User command: /message /msg

Does that work? I think it's what Martin landed on

@rgee0
Copy link
Contributor

rgee0 commented Jul 7, 2019

Using message as the command wasn't mentioned once on #109.

@martindekov if this is meant to be a separate feature then there's further rework required. If I ask how a user would use the message command without enabling comments then that should get you thinking in the right area.

@alexellis
Copy link
Owner

You're right. I think this doesn't need to be a toggle, but the feature should be documented with the names as per above.

@alexellis
Copy link
Owner

You are correct again, I suggested "topic" as the command, but Martin chose "message" and I'm ok with that choice.

@martindekov
Copy link
Sponsor Contributor Author

What I might have seen that somewhere else not sure why I got message in my head I apologise I will swap that for sure.

@martindekov
Copy link
Sponsor Contributor Author

I will go with topic

@alexellis
Copy link
Owner

I preferred your suggestion of /message and /msg. Please can you keep that as it is?

@alexellis
Copy link
Owner

To clarify: #109 (comment)

Adding messages option to the derek bot in order
to apply tempaltes or boilerplate things that we write
all the time.

Signed-off-by: Martin Dekov (VMware) <mdekov@vmware.com>
@martindekov martindekov force-pushed the martindekov/109_add_messages branch from 2857122 to edf97f0 Compare July 7, 2019 14:22
@martindekov
Copy link
Sponsor Contributor Author

Thank you @rgee0 for the extended review. I will need to test the change before it is good to go. If you have any more comments let me know. 💯

@martindekov
Copy link
Sponsor Contributor Author

Also tested e2e on my repo I believe if there are no further commends this is good to go 👍

Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved

@alexellis alexellis merged commit dd12614 into alexellis:master Jul 21, 2019
@alexellis
Copy link
Owner

Merged 👍 thank you Martin

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.

Help topics for Derek
3 participants