#1039 Migrate azure/go-amqp to version 1.0.5 #1040

cx-joses · 2024-04-24T13:50:13Z

Changes:

migrate dependent packages to the latest version
migrate azure/go-amqp from 0.17 to 1.0.5

embano1 · 2024-04-24T15:03:56Z

Thx for the quick fix! Question: is the reason to bump to go 1.22 due to the amqp bump or actually not needed?

embano1 · 2024-04-24T15:06:37Z

protocol/amqp/v2/receiver.go

-type receiver struct{ amqp *amqp.Receiver }
+type receiver struct {
+	amqp    *amqp.Receiver
+	options *amqp.ReceiveOptions


question: do we need pointer semantics here (and in the constructors)?

if you notice the changes on sender file require a pointer since the library used assumes that if nil is passed default is assumed. If in the future the library does the same for the receiver it will be prepared to it. WDYT?

That is fine, you can always use the copied value with &var - but make a copy of the provided struct (to avoid races/bugs) when calling New...()

sure, gone back to not using pointer for this case

cx-joses · 2024-04-24T15:17:05Z

Thx for the quick fix! Question: is the reason to bump to go 1.22 due to the amqp bump or actually not needed?

@embano1 actually it was not supposed to be done. I was thinking of upgrading it all but there is no need and required more changes.

embano1 · 2024-04-24T15:42:44Z

Thx, I still see the bumps though?

cx-joses · 2024-04-24T16:04:41Z

Thx, I still see the bumps though?

@embano1 I actually upgraded to 1.21 since its the version referred on build steps and others. do you see any issue in doing so?

embano1 · 2024-04-24T18:32:35Z

@embano1 I actually upgraded to 1.21 since its the version referred on build steps and others. do you see any issue in doing so?

IMHO we want to keep 1.18 so we don't introduce additional changes/features that might block users compiling with older go versions. The README states:

Note: Supported go version: 1.18+

cx-joses · 2024-04-24T18:38:47Z

@embano1 I actually upgraded to 1.21 since its the version referred on build steps and others. do you see any issue in doing so?

IMHO we want to keep 1.18 so we don't introduce additional changes/features that might block users compiling with older go versions. The README states:

Note: Supported go version: 1.18+

understood, you are absolutely right! reverted it all to 1.18 again! thanks

embano1 · 2024-04-24T18:50:34Z

samples/amqp/receiver/main.go

 	}
 	return env, strings.TrimPrefix(u.Path, "/"), opts
 }

 func main() {
 	host, node, opts := sampleConfig()
-	p, err := ceamqp.NewProtocol(host, node, []amqp.ConnOption{}, []amqp.SessionOption{}, opts...)
+	p, err := ceamqp.NewProtocol(context.Background(), host, node, opts, &amqp.SessionOptions{},
+		&amqp.SenderOptions{}, &amqp.ReceiverOptions{})


would be much nicer if those would not be pointers IMHO

made it use the struct instead of pointers. thanks

protocol/amqp/v2/protocol.go

embano1 · 2024-04-27T06:24:54Z

protocol/amqp/v2/sender.go

-	for _, o := range options {
-		o(s)
-	}
+func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender {


nit: inconsistent with other functions due to pointer

Suggested change

func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender {

func NewSender(amqpSender *amqp.Sender, options amqp.SendOptions) protocol.Sender {

the thing here is that internally there is a default behavior assumed for a nil passing. from there I am assuming a pointer here. is it wrong?

The Receiver does not use this though (uses struct semantics). Is this expected? If passing nil is valid we

either need to document this or

use a functional options pattern where we have nil as default behavior

embano1 · 2024-04-27T06:25:28Z

samples/amqp/receiver/main.go

 	}
 	return env, strings.TrimPrefix(u.Path, "/"), opts
 }

 func main() {
 	host, node, opts := sampleConfig()
-	p, err := ceamqp.NewProtocol(host, node, []amqp.ConnOption{}, []amqp.SessionOption{}, opts...)
+	p, err := ceamqp.NewProtocol(context.Background(), host, node, opts, amqp.SessionOptions{},
+		amqp.SenderOptions{}, amqp.ReceiverOptions{})


interesting: so we don't need those options, which goes back to my comment on keeping functional options pattern

embano1 · 2024-04-27T06:26:15Z

samples/amqp/sender/main.go

@@ -51,7 +53,8 @@ type Example struct {

 func main() {
 	host, node, opts := sampleConfig()
-	p, err := ceamqp.NewProtocol(host, node, []amqp.ConnOption{}, []amqp.SessionOption{}, opts...)
+	p, err := ceamqp.NewProtocol(context.Background(), host, node, opts, amqp.SessionOptions{}, amqp.SenderOptions{},


see other comment on options

embano1 · 2024-04-27T06:27:06Z

test/integration/amqp/amqp_test.go

 	require.NoError(t, err)
-	return client, session, addr
+	senderOpts = amqp.SenderOptions{}
+	require.NotNil(t, senderOpts)


what is this testing?

embano1 · 2024-04-27T06:28:24Z

test/integration/amqp_binding/amqp_test.go

+	require.NotNil(t, senderOpts)
+	receiverOpts = &amqp.ReceiverOptions{}
+	require.NotNil(t, receiverOpts)
+	return client, session, addr, senderOpts, receiverOpts


nit: unless needed, don't return pointers but structs

in the other function you used structs on return

mschneider82 · 2024-07-09T12:34:57Z

any change that this will be merged soon? using the stable go-amqp version would be nice.

embano1 · 2024-07-09T15:54:07Z

@cx-joses are you still down to finish this one?

cx-joses · 2024-07-10T15:49:32Z

@cx-joses are you still down to finish this one?

I am, I had vacations and another internal topics that made this pending but I will pick it up. Thanks for the notice!

duglin · 2024-07-17T20:26:42Z

@cx-joses not to put pressure on you :-) but this was identified as one of the ones we'd like to get merged before we cut a new release...soon-ish. If you don't think you'll have time to rebase and address Michael's comments soon please let us know

cx-joses · 2024-07-18T13:20:44Z

@duglin what is the timeline you guys are expecting? sorry for the lack of time on this, this is a priority for me also but came in a difficult time! Let's see if I find the time tomorrow to work on this

duglin · 2024-07-18T14:03:23Z

@cx-joses I don't think there's a firm date, just people getting antsy because of all of the new stuff that's gone in and in particular the release of the CESQL v1.0 spec - which the SDK now supports.

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

cx-joses · 2024-07-19T08:40:18Z

@cx-joses I don't think there's a firm date, just people getting antsy because of all of the new stuff that's gone in and in particular the release of the CESQL v1.0 spec - which the SDK now supports.

sure, I will make the comments changes today!

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

protocol/amqp/v2/protocol.go

embano1 · 2024-07-20T06:41:55Z

protocol/amqp/v2/sender.go

-	for _, o := range options {
-		o(s)
-	}
+func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender {


The Receiver does not use this though (uses struct semantics). Is this expected? If passing nil is valid we

either need to document this or

use a functional options pattern where we have nil as default behavior

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

embano1 · 2024-07-28T07:55:58Z

protocol/amqp/v2/options.go

@@ -12,42 +12,76 @@ import (
 // Option is the function signature required to be considered an amqp.Option.
 type Option func(*Protocol) error

-// WithConnOpt sets a connection option for amqp
-func WithConnOpt(opt amqp.ConnOption) Option {
+type SendOption func(sender *sender)


nit: both need doc strings

embano1 · 2024-07-28T07:57:37Z

protocol/amqp/v2/options.go

 		return nil
 	}
 }

+// WithReceiveOpts sets a receive option for amqp


Can you please explain why we need WithReceiverOpts and WithReceiveOpts (also for send)? Is there a way to only have one? These changes complicate the API and I'm having a hard time understanding why we need those very similar options (and I guess new CE users would also be slightly confused?)

IIUC, this is for the sender/receiver construction, which currently does not support options. Do we need this new functionality at the cost of a more complex user API?

embano1 · 2024-07-28T08:00:27Z

protocol/amqp/v2/options.go

+type ReceiveOption func(receiver *receiver)
+
+// WithConnOpts sets a connection option for amqp
+func WithConnOpts(opt *amqp.ConnOptions) Option {


Since you're using pointer semantics in those options, be careful with races - users might not be aware that you're passing the pointer around (instead of copy) and this could lead to surprises for users. I suggest using value semantic to avoid races and these surprises.

duglin · 2024-09-26T14:08:27Z

@cx-joses what's the status of this one?

cx-joses requested a review from a team as a code owner April 24, 2024 13:50

cx-joses force-pushed the main branch from 7fa79fe to 09780aa Compare April 24, 2024 13:52

cx-joses mentioned this pull request Apr 24, 2024

Support https://github.com/Azure/go-amqp stable version #1039

Open

cx-joses force-pushed the main branch 7 times, most recently from c7b277f to 7e99825 Compare April 24, 2024 15:02

embano1 reviewed Apr 24, 2024

View reviewed changes

cx-joses force-pushed the main branch from 7e99825 to b8c6112 Compare April 24, 2024 15:17

cx-joses requested a review from embano1 April 24, 2024 17:11

cx-joses changed the title ~~#1039 - upgrade go to 1.22; upgrade packages; migrate azure/go-amqp to 1.0.5~~ #1039 Migrate azure/go-amqp to version 1.0.5 Apr 24, 2024

cx-joses force-pushed the main branch from b8c6112 to dd95544 Compare April 24, 2024 18:38

embano1 reviewed Apr 24, 2024

View reviewed changes

cx-joses force-pushed the main branch from dd95544 to 9b65c4f Compare April 24, 2024 19:17

cx-joses requested a review from embano1 April 24, 2024 19:33

cx-joses force-pushed the main branch from 9b65c4f to 4c2a141 Compare April 25, 2024 14:16

embano1 requested changes Apr 27, 2024

View reviewed changes

upgrade go to 1.22; upgrade packages; migrate azure/go-amqp to 1.0.5

f7c544c

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

cx-joses force-pushed the main branch from 4c2a141 to f7c544c Compare July 19, 2024 08:38

reapply options pattern

d173978

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

cx-joses force-pushed the main branch from d024c52 to d173978 Compare July 19, 2024 17:41

cx-joses requested a review from embano1 July 19, 2024 17:41

embano1 reviewed Jul 20, 2024

View reviewed changes

cx-joses requested a review from embano1 July 22, 2024 10:57

cx-joses force-pushed the main branch 3 times, most recently from 5231be7 to 1743a2f Compare July 22, 2024 13:46

apply functional pattern to receive and send

0480cd0

Signed-off-by: Jose Silva <jose.silva@checkmarx.com>

cx-joses force-pushed the main branch from 1743a2f to 0480cd0 Compare July 22, 2024 13:56

embano1 reviewed Jul 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1039 Migrate azure/go-amqp to version 1.0.5 #1040

#1039 Migrate azure/go-amqp to version 1.0.5 #1040

cx-joses commented Apr 24, 2024 •

edited

Loading

embano1 commented Apr 24, 2024

embano1 Apr 24, 2024

cx-joses Apr 24, 2024

embano1 Apr 24, 2024

cx-joses Apr 24, 2024

cx-joses commented Apr 24, 2024 •

edited

Loading

embano1 commented Apr 24, 2024

cx-joses commented Apr 24, 2024

embano1 commented Apr 24, 2024

cx-joses commented Apr 24, 2024

embano1 Apr 24, 2024

cx-joses Apr 24, 2024

embano1 Apr 27, 2024

cx-joses Jul 19, 2024

embano1 Jul 20, 2024

embano1 Apr 27, 2024

embano1 Apr 27, 2024

embano1 Apr 27, 2024

embano1 Apr 27, 2024

embano1 Apr 27, 2024

mschneider82 commented Jul 9, 2024

embano1 commented Jul 9, 2024

cx-joses commented Jul 10, 2024

duglin commented Jul 17, 2024

cx-joses commented Jul 18, 2024

duglin commented Jul 18, 2024

cx-joses commented Jul 19, 2024

embano1 Jul 20, 2024

embano1 Jul 28, 2024

embano1 Jul 28, 2024

embano1 Jul 28, 2024

embano1 Jul 28, 2024

duglin commented Sep 26, 2024

	func NewSender(amqpSender amqp.Sender, options amqp.SendOptions) protocol.Sender {
	func NewSender(amqpSender *amqp.Sender, options amqp.SendOptions) protocol.Sender {

#1039 Migrate azure/go-amqp to version 1.0.5 #1040

Are you sure you want to change the base?

#1039 Migrate azure/go-amqp to version 1.0.5 #1040

Conversation

cx-joses commented Apr 24, 2024 • edited Loading

embano1 commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cx-joses commented Apr 24, 2024 • edited Loading

embano1 commented Apr 24, 2024

cx-joses commented Apr 24, 2024

embano1 commented Apr 24, 2024

cx-joses commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschneider82 commented Jul 9, 2024

embano1 commented Jul 9, 2024

cx-joses commented Jul 10, 2024

duglin commented Jul 17, 2024

cx-joses commented Jul 18, 2024

duglin commented Jul 18, 2024

cx-joses commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented Sep 26, 2024

cx-joses commented Apr 24, 2024 •

edited

Loading

cx-joses commented Apr 24, 2024 •

edited

Loading