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
Make ALB event path optional #8571
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8571 +/- ##
==========================================
+ Coverage 86.93% 86.94% +0.01%
==========================================
Files 254 252 -2
Lines 9518 9515 -3
==========================================
- Hits 8274 8273 -1
+ Misses 1244 1242 -2
Continue to review full report at Codecov.
|
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.
@jinhong- Great thanks for this PR! Still before I review, let's first agree on right solution in the issue (I've posted the comment there)
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.
@jinhong- Thank you! It looks very good.
Still we've taken a new better approach to testing in last months, and all new tests should follow that. Please check my comment, and let me know if anything is not clear enough
test/unit/lib/plugins/aws/package/compile/events/alb/lib/listenerRules.test.js
Outdated
Show resolved
Hide resolved
test/unit/lib/plugins/aws/package/compile/events/alb/lib/validate.test.js
Outdated
Show resolved
Hide resolved
@medikoo Reverted old tests and updated new tests |
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.
Thank you @jinhong- Looks good, I've just proposed few organization improvements
test/unit/lib/plugins/aws/package/compile/events/alb/index.test.js
Outdated
Show resolved
Hide resolved
test/unit/lib/plugins/aws/package/compile/events/alb/index.test.js
Outdated
Show resolved
Hide resolved
test/unit/lib/plugins/aws/package/compile/events/alb/index.test.js
Outdated
Show resolved
Hide resolved
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.
Thank you @jinhong- ! It looks very good. I've proposed just few last improvements
test/unit/lib/plugins/aws/package/compile/events/alb/index.test.js
Outdated
Show resolved
Hide resolved
test/unit/lib/plugins/aws/package/compile/events/alb/index.test.js
Outdated
Show resolved
Hide resolved
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.
Thank you @jinhong- ! It looks great, let's just improve error code
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.
Thank you @jinhong- !
You're most welcome :) |
Closes: #8568
ALB event path should have been optional. However omission of the path would result in a failure in deployment. This PR addresses the issue by excluding the path when it is not set