#1039 Migrate azure/go-amqp to version 1.0.5 #1040
Conversation
cx-joses
force-pushed
the
main
branch
7 times, most recently
from
c7b277f
to
7e99825
Compare
|
Thx for the quick fix! Question: is the reason to bump to |
protocol/amqp/v2/receiver.go
Outdated
| type receiver struct{ amqp *amqp.Receiver } | ||
| type receiver struct { | ||
| amqp *amqp.Receiver | ||
| options *amqp.ReceiveOptions |
There was a problem hiding this comment.
question: do we need pointer semantics here (and in the constructors)?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...()
There was a problem hiding this comment.
sure, gone back to not using pointer for this case
@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. |
|
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
changed the title
IMHO we want to keep
|
understood, you are absolutely right! reverted it all to 1.18 again! thanks |
samples/amqp/receiver/main.go
Outdated
| } | ||
| 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{}) |
There was a problem hiding this comment.
would be much nicer if those would not be pointers IMHO
There was a problem hiding this comment.
made it use the struct instead of pointers. thanks
Signed-off-by: Jose Silva <jose.silva@checkmarx.com>
| @@ -35,54 +30,60 @@ type Protocol struct { | |||
| } | |||
|
|
|||
| // NewProtocolFromClient creates a new amqp transport. | |||
| func NewProtocolFromClient(client *amqp.Client, session *amqp.Session, queue string, opts ...Option) (*Protocol, error) { | |||
| func NewProtocolFromClient( | |||
There was a problem hiding this comment.
question: are we ok with this breaking change on this signature? I understand why we technically might not need options anymore (since those are now structs), but you might want to keep the options pattern to not break this signature and allow for future expansions. You could introduce a new option to set sender/receiver options on the protocol.
There was a problem hiding this comment.
Or are you worried that those two structs are always needed and thus an options pattern doesn't really apply?
| return t, nil | ||
| } | ||
|
|
||
| // NewProtocol creates a new amqp transport. | ||
| func NewProtocol(server, queue string, connOption []amqp.ConnOption, sessionOption []amqp.SessionOption, opts ...Option) (*Protocol, error) { | ||
| client, err := amqp.Dial(server, connOption...) | ||
| func NewProtocol( |
| for _, o := range options { | ||
| o(s) | ||
| } | ||
| func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender { |
There was a problem hiding this comment.
nit: inconsistent with other functions due to pointer
| func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender { | |
| func NewSender(amqpSender *amqp.Sender, options amqp.SendOptions) protocol.Sender { |
| } | ||
| 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{}) |
There was a problem hiding this comment.
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{}, | |||
| require.NoError(t, err) | ||
| return client, session, addr | ||
| senderOpts = amqp.SenderOptions{} | ||
| require.NotNil(t, senderOpts) |
| require.NotNil(t, senderOpts) | ||
| receiverOpts = &amqp.ReceiverOptions{} | ||
| require.NotNil(t, receiverOpts) | ||
| return client, session, addr, senderOpts, receiverOpts |
There was a problem hiding this comment.
nit: unless needed, don't return pointers but structs
There was a problem hiding this comment.
in the other function you used structs on return
Changes:
Fixes #1039