Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48
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
base: master
Are you sure you want to change the base?
Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48
Changes from all commits
61a6061
58a1f62
781ab4d
0e3212f
1d7b3e8
35ec697
7466939
d9ebb41
a051f8f
e94b229
d88f34c
df5bc43
4b85204
40c3caa
8cc9d40
8c7ae21
4de87b6
2476701
1ac18e8
705bdde
786f90b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think this test is actually validating that the AwsTgwConnector endpoint didn't get added. You should loop through the compositeEndpoints to check each element to ensure it's not AwsTgwConnector
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, will do.
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 comment re: looping through the compositeEndpoints.
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.
Will do
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.
these 2 cases are good candidates to convert to a table driven approach
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 will change it to a table driven test
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.
are these golint related fixes? if there's a lot of these please add these as a separate PR
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.
When I tried running the unit tests I wrote for aws_tgw_connect.go there was a build failure for this file that was preventing my tests from running. Failure happened due to having the %w instead of %v.
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.
Just noting here for exaplanation:
This most likely a forgotten formatting directive that was probably copy-pasted from other
fmt.Errorf
call. The%w
is a special specified for errors, read more here: https://golang.org/pkg/fmt/#Errorf Since this islogrus.Errorf
that just prints log and does not create error it is incorrect in this case.