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

removed notifications/slack package [Merge after 1.7.0 release] #368

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

atighineanu
Copy link
Contributor

In this PR the slack-hook-url is translated
into shoutrrr syntax. Therefore, slack pack
age as well as checks for slack-hook-url in
drain and reboot functions are removed.

I suppose we will keep the --slack-hook-url for an undefined amount of releases. Therefore, I kept it while I got rid of the notifications/slack package.

@atighineanu atighineanu changed the title removed notifications/slack package removed notifications/slack package [do not merge until 1.7.0!] Apr 29, 2021
@atighineanu atighineanu changed the title removed notifications/slack package [do not merge until 1.7.0!] removed notifications/slack package [Merge after 1.7.0 release] Apr 29, 2021
@dholbach dholbach added this to the 1.8.0 milestone May 26, 2021
@evrardjp
Copy link
Collaborator

Look at all this red (in commit diff), and all this green (in CI). I vote for that!
Can you rebase it, please?

cmd/kured/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

Needs a few changes and a rebase :)

cmd/kured/main.go Outdated Show resolved Hide resolved
@github-actions
Copy link

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

@atighineanu
Copy link
Contributor Author

  1. Unit test for flagCheck() function is added.
  2. The code is improved (used net/url and strings.Trim instead of stupidly splitting pieces of the URL).

@evrardjp
Copy link
Collaborator

evrardjp commented Oct 5, 2021

That diff looks good. I will have a view in more details soon.

Copy link
Collaborator

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

Up for discussion :)

cmd/kured/main.go Show resolved Hide resolved
@atighineanu atighineanu force-pushed the proto_removed_slack branch 4 times, most recently from 9a5f281 to 2588b6c Compare October 7, 2021 07:08
 In this PR the slack-hook-url is translated
 into shoutrrr syntax. Therefore, slack pack
 age as well as checks for slack-hook-url in
 drain and reboot functions are removed.
 Also added a unit test for flagCheck(), this
 function also checks the (slack)URL syntax.
Copy link
Member

@ckotzbauer ckotzbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@ckotzbauer ckotzbauer modified the milestones: 1.8.0, 1.8.1 Oct 8, 2021
Copy link
Collaborator

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@dholbach
Copy link
Member

@evrardjp Are you happy with this now?

@jackfrancis
Copy link
Collaborator

Revisited this PR, it looks good for merge from my perspective.

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.

Deprecated flag(s). Please use --notify-url flag instead.
5 participants