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

Jump to bottom

[experimental] introduction of Slack workflow steps #1027

Merged
merged 35 commits into from Mar 11, 2022
Merged

[experimental] introduction of Slack workflow steps #1027

merged 35 commits into from Mar 11, 2022

Conversation

SteffenMahler
Copy link

New experimental feature "Slack workflow steps" added

  • make pr-prep
  • tests included
  • dependency github.com/google/go-cmp/cmp for test added
  • example App included

@kanata2
Copy link
Member

kanata2 commented Feb 8, 2022 •
edited

Thanks! I'll confirm later 👀

BTW, please use workflow_step instead of workflowStep in dir name and file name.

Steffen Mahler and others added 6 commits February 8, 2022 07:57
using snake case for new directory and file
0a59db7
Add refresh_token and token_type to OAuthV2Response fields
4d6e826
@kanata2
Merge pull request slack-go#1030 from hidenami-i/master
9cad061 
Add refresh_token and token_type to OAuthV2Response fields
@zchee
all: add new //go:build lines
7f6b224 
$ go version
go version go1.17.7 darwin/amd64

$ make fmt

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
all: remove github.com/pkg/errors dependency
eaf6740 
The github.com/pkg/errors package has been deprecated.

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@kanata2
Merge pull request slack-go#1032 from zchee/go117-buildtag
1bc0bcc 
all: add new //go:build lines
@kanata2
Copy link
Member

kanata2 commented Feb 13, 2022

@SteffenMahler
Could you confirm this?
https://github.com/slack-go/slack/runs/5167540820

@kanata2 kanata2 added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Feb 13, 2022
zchee and others added 2 commits February 13, 2022 22:00
@zchee
vendor: run go mod vendor
86cef73 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@kanata2
Merge pull request slack-go#1033 from zchee/remove-pkg-errors-deps
bbab81d 
all: remove github.com/pkg/errors dependency
@SteffenMahler
Copy link
Author

@kanata2
Yes, I will check the linter problem soon and let you know when I am done.

zchee and others added 4 commits February 14, 2022 20:52
@zchee
messageID: add benchmark for NewSafeID
c01cb38 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
messageID: optimize NewSafeID using atomic instead of mutex lock
fb30ed3 
name                  old time/op    new time/op    delta
NewSafeID-20            13.9ns ± 2%     7.5ns ± 1%  -46.16%  (p=0.008 n=5+5)
NewSafeIDParallel-20    24.2ns ± 6%    22.1ns ± 1%   -9.06%  (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
messageID: add NewSafeID testcase
1265b2e 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
switch go code style for imports from gofmt to goimports
af5cf8b
@SteffenMahler
Copy link
Author

Hello @kanata2

I have switched to goimports for sorting and formatting of imports. So the linter check should be successful now.
Maybe we should integrate docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.44.0 golangci-lint run -v into the makefile?
Could you be so kind as to start the action again?

Thank you.

@zchee
all: support pass context.Context to all methods
0389f44 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
webhooks: remove go1.12 support
dacbdf8 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
misc: use NewRequestWithContext
d89251b 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
misc: use http.MethodXXX constant
1645ac2 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
messageID: fix atomic operation suggested by brainexe
daa4329 
name                  old time/op    new time/op    delta
NewSafeID-20            7.60ns ± 1%    5.93ns ± 1%  -21.97%  (p=0.008 n=5+5)
NewSafeIDParallel-20    21.0ns ± 1%    21.0ns ± 2%     ~     (p=0.952 n=5+5)

name                  old alloc/op   new alloc/op   delta
NewSafeID-20             0.00B          0.00B          ~     (all equal)
NewSafeIDParallel-20     8.00B ± 0%     8.00B ± 0%     ~     (all equal)

name                  old allocs/op  new allocs/op  delta
NewSafeID-20              0.00           0.00          ~     (all equal)
NewSafeIDParallel-20      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

See also:
- slack-go#1035 (review)

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
messageID: add documentation
93ff3fb 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
chat: add some BuildRequestContext methods for backwards compatibility
67b468f 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
webhook: remove unnecessary PostWebhookContextCustomHTTP function
741a1f5 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee zchee requested review from zchee and kanata2 February 24, 2022 14:25
@zchee
Merge pull request slack-go#1034 from zchee/ctx-aware
ff619ea 
Support pass context.Context to all methods
@zchee
Merge pull request slack-go#1035 from zchee/opt-new-safe-id
f6658aa 
Optimize NewSafeID using atomic
@zchee
Copy link
Contributor

zchee commented Feb 26, 2022

@SteffenMahler Hi, this is newer slack-go maintainer.

The webhooks_go112.go and webhooks_go113.go files has been removed (my previous pull request). Could you rebase it?

As you suggested:

Maybe we should integrate docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.44.0 golangci-lint run -v into the makefile?

Yeah, our GitHub Actions policy's don't running until accepted by any maintainers (I hope it's a commonly used policy). So contributor can't checks whether the will fail lint job or etc.
Make sense, I'll consider adding it. Thanks.

@SteffenMahler
Copy link
Author

Hi @zchee,

I don't know what I did wrong. Now there are changes in my branch that I didn't made and should already be in the main branch. Can you have a look?

@zchee
Copy link
Contributor

zchee commented Feb 28, 2022

@SteffenMahler Sure, will review soon.

@victorboissiere
Copy link

FYI, we are currently using your branch in our internal project. This is working perfectly & deployed into production. Really much needed.

@zchee
Copy link
Contributor

zchee commented Mar 2, 2022

@victorboissiere Thanks for great feedback. We should merge ASAP anyway.

zchee
zchee approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@zchee zchee left a comment

Choose a reason for hiding this comment

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

LGTM

@zchee zchee merged commit b5149b1 into slack-go:master Mar 11, 2022
@zchee zchee mentioned this pull request Mar 11, 2022
Merged
@kanata2 kanata2 removed needs review feedback given When a review has been conducted and awaiting the response from the comitter(s) labels Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zchee zchee zchee approved these changes

@kanata2 kanata2 Awaiting requested review from kanata2

Assignees
No one assigned
Labels
enhancement experimental experimental feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SteffenMahler @kanata2 @zchee @victorboissiere