-
Notifications
You must be signed in to change notification settings - Fork 56
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 basic support for notifications package #160
Add basic support for notifications package #160
Conversation
Welcome @Devaansh-Kumar! |
Hi @Devaansh-Kumar. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this first PR, @Devaansh-Kumar!
/ok-to-test
t := newTableConfig() | ||
|
||
t.SetTitle(fmt.Sprintf("Notifications from %v", provider)) | ||
t.AppendHeader(table.Row{"Provider", "Message Type", "Notification"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd stick with either the "message" or the "notification" naming, which means "Notification type | notification" or "message type | message".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with changing the Heading "Message Type" to "Notification Type", but withing the Notification
struct I cannot change Message
field to Notification
as there would be a name clash and Notification
struct consists of both a Type and a message that we want to send.
Do you thing I should change MessageType
to NotificationType
as well?
85783e5
to
5aa3115
Compare
@mlavacca I have made changes according to your comments and left some comments of my own and am ready for next round of review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncomfortable to approve this CL because I want to see the whole picture of how the notifications will be added, then I can have an opinion on the structure introduced here as well.
Can you add the other part of the logic here ?
To make review easy, you can logically split it to commits instead of separate PRs.
pkg/i2gw/notifications/config.go
Outdated
"github.com/jedib0t/go-pretty/v6/table" | ||
"github.com/jedib0t/go-pretty/v6/text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful importing these non standard libraries, are there more common alternatives ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a well used library, so it should not be a concern. One alternative would be https://github.com/olekukonko/tablewriter but i feel the package we are using offers more flexibility and options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok about this, if it was an APACHE licenses it was all good.
I checked and it looks like CNCF has a process for allowlisting MIT licenses. https://github.com/olekukonko/tablewriter is allowed, you can find it here.
The approval processes would probably take a bit of time, and has some requirements, here is an example of exception request.
Up to you if you want to go through this process, or move to use github.com/olekukonko/tablewriter
and remove the colors. I'd go with the latter.
By other part you mean the part involving having a calling object as a column right? |
No, I am fine if you add that column later (in a different commit or PR).
I mean the part where providers use this package. Currently no one is
adding or removing notifications IIRC.
…On Sun, 16 Jun 2024 at 14:20, Devaansh Kumar ***@***.***> wrote:
I am uncomfortable to approve this CL because I want to see the whole
picture of how the notifications will be added, then I can have an opinion
on the structure introduced here as well.
Can you add the other part of the logic here ? To make review easy, you
can logically split it to commits instead of separate PRs.
By other part you mean the part involving having a calling object as a
column right?
—
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASLANQXH5PP6K2ZP6K65TPLZHWGLFAVCNFSM6AAAAABI5JWDWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZRGYYDCNJTGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you Devaansh!
Mostly LGTM, I left a few last comments, plus can you add a test to notifications.go ? to see that we get the tables we expect?
If its too hard for this PR, I am happy to wait for a followup PR but if its straightforward i'd prefer to include it here.
Yeah I would prefer to do this in a seperate PR. |
e852163
to
614b901
Compare
614b901
to
757a826
Compare
e3dbb65
to
b0c1ed1
Compare
b0c1ed1
to
d65bc3e
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Devaansh-Kumar, LiorLieberman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@Devaansh-Kumar can you rebase? |
d65bc3e
to
d579fa7
Compare
/lgtm |
What type of PR is this?
/kind feature
/kind documentation
What this PR does / why we need it:
This PR provides a skeleton structure for the notifications package
Which issue(s) this PR fixes:
This pull request addresses part of the issue described in #131
Does this PR introduce a user-facing change?:
/cc @LiorLieberman @mlavacca