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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cx-joses
Copy link

@cx-joses cx-joses commented Apr 24, 2024

Changes:

Fixes #1039

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

@embano1
Copy link
Member

embano1 commented Apr 24, 2024

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

type receiver struct{ amqp *amqp.Receiver }
type receiver struct {
amqp *amqp.Receiver
options *amqp.ReceiveOptions
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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...()

Copy link
Author

Choose a reason for hiding this comment

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

sure, gone back to not using pointer for this case

@cx-joses
Copy link
Author

cx-joses commented Apr 24, 2024

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
Copy link
Member

embano1 commented Apr 24, 2024

Thx, I still see the bumps though?

@cx-joses
Copy link
Author

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?

@cx-joses cx-joses requested a review from embano1 April 24, 2024 17:11
@cx-joses 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
@embano1
Copy link
Member

embano1 commented Apr 24, 2024

@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
Copy link
Author

@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

}
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{})
Copy link
Member

Choose a reason for hiding this comment

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

would be much nicer if those would not be pointers IMHO

Copy link
Author

Choose a reason for hiding this comment

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

made it use the struct instead of pointers. thanks

protocol/amqp/v2/protocol.go Show resolved Hide resolved
protocol/amqp/v2/protocol.go Show resolved Hide resolved
for _, o := range options {
o(s)
}
func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender {
Copy link
Member

Choose a reason for hiding this comment

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

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 {

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. either need to document this or
  2. use a functional options pattern where we have nil as default behavior

}
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{})
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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{},
Copy link
Member

Choose a reason for hiding this comment

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

see other comment on options

require.NoError(t, err)
return client, session, addr
senderOpts = amqp.SenderOptions{}
require.NotNil(t, senderOpts)
Copy link
Member

Choose a reason for hiding this comment

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

what is this testing?

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

in the other function you used structs on return

@mschneider82
Copy link

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

@embano1
Copy link
Member

embano1 commented Jul 9, 2024

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

@cx-joses
Copy link
Author

@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
Copy link
Contributor

duglin commented Jul 17, 2024

@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
Copy link
Author

@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
Copy link
Contributor

duglin commented Jul 18, 2024

@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
Copy link
Author

@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 Show resolved Hide resolved
protocol/amqp/v2/protocol.go Show resolved Hide resolved
for _, o := range options {
o(s)
}
func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender {
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. either need to document this or
  2. use a functional options pattern where we have nil as default behavior

@cx-joses cx-joses requested a review from embano1 July 22, 2024 10:57
@cx-joses cx-joses force-pushed the main branch 3 times, most recently from 5231be7 to 1743a2f Compare July 22, 2024 13:46
Signed-off-by: Jose Silva <jose.silva@checkmarx.com>
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

nit: both need doc strings

return nil
}
}

// WithReceiveOpts sets a receive option for amqp
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

type ReceiveOption func(receiver *receiver)

// WithConnOpts sets a connection option for amqp
func WithConnOpts(opt *amqp.ConnOptions) Option {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor

duglin commented Sep 26, 2024

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

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.

Support https://github.com/Azure/go-amqp stable version
4 participants