Add basic support for notifications package #160

Devaansh-Kumar · 2024-06-06T18:59:13Z

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?:

Created a new notifications package to provide mechanism for providers to display useful information generated during conversion process

/cc @LiorLieberman @mlavacca

k8s-ci-robot · 2024-06-06T18:59:21Z

Welcome @Devaansh-Kumar!

It looks like this is your first PR to kubernetes-sigs/ingress2gateway 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/ingress2gateway has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-06-06T18:59:22Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

mlavacca

Thanks for this first PR, @Devaansh-Kumar!

/ok-to-test

pkg/i2gw/notifications/notifications.go

mlavacca · 2024-06-12T13:39:14Z

pkg/i2gw/notifications/notifications.go

+		t := newTableConfig()
+
+		t.SetTitle(fmt.Sprintf("Notifications from %v", provider))
+		t.AppendHeader(table.Row{"Provider", "Message Type", "Notification"})


I'd stick with either the "message" or the "notification" naming, which means "Notification type | notification" or "message type | message".

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?

pkg/i2gw/notifications/notifications.go

pkg/i2gw/notifications/config.go

Devaansh-Kumar · 2024-06-13T18:23:15Z

@mlavacca I have made changes according to your comments and left some comments of my own and am ready for next round of review

PROVIDER.md

LiorLieberman

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.

LiorLieberman · 2024-06-15T18:15:02Z

pkg/i2gw/notifications/config.go

+	"github.com/jedib0t/go-pretty/v6/table"
+	"github.com/jedib0t/go-pretty/v6/text"


We should be careful importing these non standard libraries, are there more common alternatives ?

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.

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.

pkg/i2gw/notifications/config.go

Devaansh-Kumar · 2024-06-16T13:20:28Z

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?

LiorLieberman · 2024-06-16T14:17:20Z

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: ***@***.***>

pkg/i2gw/ingress2gateway.go

pkg/i2gw/notifications/notifications.go

pkg/i2gw/providers/istio/converter.go

LiorLieberman

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.

pkg/i2gw/notifications/config.go

pkg/i2gw/notifications/notifications.go

pkg/i2gw/providers/istio/converter.go

Devaansh-Kumar · 2024-06-28T13:59:10Z

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.

pkg/i2gw/notifications/config.go

pkg/i2gw/notifications/notifications.go

cmd/print.go

pkg/i2gw/notifications/notifications.go

cmd/print.go

pkg/i2gw/ingress2gateway.go

go.mod

LiorLieberman · 2024-07-12T10:36:05Z

/approve
/label tide/merge-method-squash

k8s-ci-robot · 2024-07-12T10:36:12Z

[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:

~~OWNERS~~ [LiorLieberman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LiorLieberman · 2024-07-12T10:37:26Z

@Devaansh-Kumar can you rebase?

LiorLieberman · 2024-07-12T15:31:23Z

/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. labels Jun 6, 2024

k8s-ci-robot requested review from levikobi and Xunzhuo June 6, 2024 18:59

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 6, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2024

mlavacca reviewed Jun 12, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 12, 2024

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from 85783e5 to 5aa3115 Compare June 13, 2024 18:18

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 13, 2024

Devaansh-Kumar requested a review from mlavacca June 15, 2024 16:43

LiorLieberman reviewed Jun 15, 2024

View reviewed changes

PROVIDER.md Outdated Show resolved Hide resolved

LiorLieberman reviewed Jun 15, 2024

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2024

Devaansh-Kumar requested a review from LiorLieberman June 24, 2024 17:41

LiorLieberman reviewed Jun 26, 2024

View reviewed changes

Devaansh-Kumar requested a review from LiorLieberman June 27, 2024 19:46

LiorLieberman reviewed Jun 28, 2024

View reviewed changes

pkg/i2gw/notifications/config.go Outdated Show resolved Hide resolved

pkg/i2gw/notifications/notifications.go Outdated Show resolved Hide resolved

pkg/i2gw/providers/istio/converter.go Show resolved Hide resolved

Devaansh-Kumar requested a review from LiorLieberman June 28, 2024 14:47

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from e852163 to 614b901 Compare June 28, 2024 20:29

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from 614b901 to 757a826 Compare June 29, 2024 08:10

LiorLieberman reviewed Jul 2, 2024

View reviewed changes

pkg/i2gw/notifications/config.go Outdated Show resolved Hide resolved

pkg/i2gw/notifications/notifications.go Outdated Show resolved Hide resolved

cmd/print.go Outdated Show resolved Hide resolved

LiorLieberman reviewed Jul 8, 2024

View reviewed changes

pkg/i2gw/notifications/notifications.go Outdated Show resolved Hide resolved

pkg/i2gw/notifications/notifications.go Outdated Show resolved Hide resolved

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from e3dbb65 to b0c1ed1 Compare July 8, 2024 18:11

LiorLieberman reviewed Jul 9, 2024

View reviewed changes

cmd/print.go Outdated Show resolved Hide resolved

pkg/i2gw/ingress2gateway.go Outdated Show resolved Hide resolved

pkg/i2gw/ingress2gateway.go Outdated Show resolved Hide resolved

pkg/i2gw/ingress2gateway.go Outdated Show resolved Hide resolved

go.mod Show resolved Hide resolved

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from b0c1ed1 to d65bc3e Compare July 9, 2024 15:49

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2024

Devaansh-Kumar added 12 commits July 12, 2024 19:02

initial setup for notifications package

e1fe7f1

created configs for table style

35005d0

created notification.go for openapi3

6f45a5c

updated documentation for the PROVIDER to use notifications package

581bc3f

shifted NotificationAggregator from slice to map

e30655b

changes to fix prow tests

4bdb866

added notifications support for Istio Virtual Services

47f018b

changes in naming convention and add locks in NotificationAggregator

4b8eb23

added comments and changed table render location

fac1e69

changed package for generating tables to olekukonko/tablewriter

c632e54

Changed table render location to string

b961ec2

edited tableString to use maps

d579fa7

Devaansh-Kumar force-pushed the Devaansh-Kumar/notification branch from d65bc3e to d579fa7 Compare July 12, 2024 13:38

Devaansh-Kumar requested a review from LiorLieberman July 12, 2024 14:49

k8s-ci-robot assigned LiorLieberman Jul 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2024

LiorLieberman merged commit 2659102 into kubernetes-sigs:main Jul 12, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for notifications package #160

Add basic support for notifications package #160

Devaansh-Kumar commented Jun 6, 2024 •

edited

Loading

k8s-ci-robot commented Jun 6, 2024

k8s-ci-robot commented Jun 6, 2024

mlavacca left a comment

mlavacca Jun 12, 2024

Devaansh-Kumar Jun 13, 2024

Devaansh-Kumar commented Jun 13, 2024

LiorLieberman left a comment

LiorLieberman Jun 15, 2024

Devaansh-Kumar Jun 16, 2024

LiorLieberman Jun 29, 2024

Devaansh-Kumar commented Jun 16, 2024

LiorLieberman commented Jun 16, 2024 via email

LiorLieberman left a comment

Devaansh-Kumar commented Jun 28, 2024

LiorLieberman commented Jul 12, 2024

k8s-ci-robot commented Jul 12, 2024

LiorLieberman commented Jul 12, 2024

LiorLieberman commented Jul 12, 2024

		"github.com/jedib0t/go-pretty/v6/table"
		"github.com/jedib0t/go-pretty/v6/text"

Add basic support for notifications package #160

Add basic support for notifications package #160

Conversation

Devaansh-Kumar commented Jun 6, 2024 • edited Loading

k8s-ci-robot commented Jun 6, 2024

k8s-ci-robot commented Jun 6, 2024

mlavacca left a comment

Choose a reason for hiding this comment

mlavacca Jun 12, 2024

Choose a reason for hiding this comment

Devaansh-Kumar Jun 13, 2024

Choose a reason for hiding this comment

Devaansh-Kumar commented Jun 13, 2024

LiorLieberman left a comment

Choose a reason for hiding this comment

LiorLieberman Jun 15, 2024

Choose a reason for hiding this comment

Devaansh-Kumar Jun 16, 2024

Choose a reason for hiding this comment

LiorLieberman Jun 29, 2024

Choose a reason for hiding this comment

Devaansh-Kumar commented Jun 16, 2024

LiorLieberman commented Jun 16, 2024 via email

LiorLieberman left a comment

Choose a reason for hiding this comment

Devaansh-Kumar commented Jun 28, 2024

LiorLieberman commented Jul 12, 2024

k8s-ci-robot commented Jul 12, 2024

LiorLieberman commented Jul 12, 2024

LiorLieberman commented Jul 12, 2024

Devaansh-Kumar commented Jun 6, 2024 •

edited

Loading