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.
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
feat(elasticloadbalancing): classic load balancer supports ec2 instances #24353
feat(elasticloadbalancing): classic load balancer supports ec2 instances #24353
Changes from 1 commit
8ad932b
e997096
39d0baa
c91d318
0b4f810
371bd14
e9f3c02
9a06351
933f6f6
38dc1f9
4015b79
c4339c5
d7190b0
bd96c01
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'd rather we take of this for the user.
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.
An
Instance
already hasConnections
, so you don't need to pass any additional ones.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 think we might need
port
as input though! How else are we going to know what port to forward to?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.
The
_
is usually used to describe a property that has to be in the function definition, but not used in the function. Usually the case is when you are implementing an inherited function but some properties in the definition are not useful for your particular implementation. Here, you are usingloadBalancer
, so there's no need to add_
as a prefix.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.
This looks like it's testing things about
SecurityGroup
, but we're not actually testing SecurityGroup here.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.
We're also not testing health checks.
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.
OK I see that
attachToClassicLB
is tested via theaddTarget
API sinceelb.addTarget
is implemented like this:TO me, it seems like
attachToClassicLB
should be the place where we implement the 'attaching' code and it's quite odd that it's empty...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.
Does this really demonstrate that an instance has been attached to a load balancer?
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.
Comment about security group should be updated to explain how the test works now.