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
base: main
Are you sure you want to change the base?
Conversation
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.
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)?
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.
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.
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...()
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.
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? |
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.
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
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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( |
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.
same question as above
for _, o := range options { | ||
o(s) | ||
} | ||
func NewSender(amqpSender *amqp.Sender, options *amqp.SendOptions) protocol.Sender { |
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.
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.
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{}, |
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.
see other comment on options
require.NoError(t, err) | ||
return client, session, addr | ||
senderOpts = amqp.SenderOptions{} | ||
require.NotNil(t, senderOpts) |
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.
what is this testing?
require.NotNil(t, senderOpts) | ||
receiverOpts = &amqp.ReceiverOptions{} | ||
require.NotNil(t, receiverOpts) | ||
return client, session, addr, senderOpts, receiverOpts |
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.
nit: unless needed, don't return pointers but structs
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.
in the other function you used structs on return
Changes:
Fixes #1039