Skip to content
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

Aws alb schema #8291

Merged
merged 27 commits into from Sep 29, 2020
Merged

Aws alb schema #8291

merged 27 commits into from Sep 29, 2020

Conversation

thewizarodofoz
Copy link
Contributor

Closes: #8020

Notes:

  1. I Left listenerArn.match(ALB_LISTENER_PATTERN) in validate.js because it's used as a parser. Feels weird importing it in awsProvider for validating but couldn't think of a better way
  2. conditions.header.name - I didn't specify the pattern, seems like an overkill
  3. healthCheck.mather.httpCode - I didn't specify the pattern, found it very hard to do

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2020

Codecov Report

Merging #8291 into master will decrease coverage by 0.04%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8291      +/-   ##
==========================================
- Coverage   88.16%   88.12%   -0.05%     
==========================================
  Files         250      250              
  Lines        9391     9365      -26     
==========================================
- Hits         8280     8253      -27     
- Misses       1111     1112       +1     
Impacted Files Coverage Δ
...ins/aws/package/compile/events/alb/lib/validate.js 95.31% <80.00%> (-2.52%) ⬇️
...ib/plugins/aws/package/compile/events/alb/index.js 100.00% <100.00%> (ø)
lib/plugins/aws/provider/awsProvider.js 93.21% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f47448...36f6b5f. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, that looks great @thewizarodofoz ! I've spotted just some minor shortcomings, please see my comments

lib/plugins/aws/package/compile/events/alb/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/alb/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/alb/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/alb/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/provider/awsProvider.js Outdated Show resolved Hide resolved
lib/plugins/aws/provider/awsProvider.js Outdated Show resolved Hide resolved
lib/plugins/aws/provider/awsProvider.js Show resolved Hide resolved
lib/plugins/aws/package/compile/events/alb/lib/validate.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/alb/lib/validate.js Outdated Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thewizarodofoz for update! Still, I see some comments not addressed/answered, have you pushed all the commits?

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thewizarodofoz, still as I see CI tests fail because regex is not right for httpCode (It has literal d instead of \d which defines digit - in a string we need to input as \\d)

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewizarodofoz looks great! I've just spotted two suggestions not answered/addressed. See my comments

lib/plugins/aws/provider/awsProvider.js Outdated Show resolved Hide resolved
lib/plugins/aws/provider/awsProvider.js Outdated Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @thewizarodofoz !

@medikoo medikoo merged commit c96b429 into serverless:master Sep 29, 2020
@thewizarodofoz thewizarodofoz deleted the aws-alb-schema branch September 29, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config schema: Define AWS "alb" function event properties
3 participants