WIP - Full CR #2
Conversation
quickliketurtle
left a comment
quickliketurtle
left a comment
There was a problem hiding this comment.
@FelipeD-FiveTalent Added comments here. Take a look and let me know if you have questions. Most are code organizations suggestions. Let me know if you need further info or if you want to work through it together.
| stage: ${opt:stage, 'dev'} | ||
| region: ${opt:region, 'us-west-2'} | ||
| environment: | ||
| env: ${self:provider.stage} |
There was a problem hiding this comment.
this is included by default in each function. no need to add explicitly.
serverless.yml
Outdated
| env: ${self:provider.stage} | ||
|
|
||
| functions: | ||
| tracker: |
There was a problem hiding this comment.
That does the name tracker represent?
There was a problem hiding this comment.
That is tracking the workflow. Dont we need to have a name for the function? should we have a different name?
There was a problem hiding this comment.
Yes you need a name, so my comment is related to it having a ligit meaning. As it's not actually tracking anything. This endpoint is listening to GitHub webhooks.
serverless.yml
Outdated
| - http: | ||
| path: tracker | ||
| method: post | ||
| cors: true |
There was a problem hiding this comment.
Is this needed? I don't think cors is used since no browser is involved.
src/index.js
Outdated
| const message = 'Done!!'; | ||
|
|
||
| callback(null, createResponse(200, { message })); |
There was a problem hiding this comment.
This shows up on github right? is there more useful information we can return? if a webhook is not processes we are still responding with done. Or if there are no commits to process, etc...
src/parseGithubWebhook.js
Outdated
| @@ -0,0 +1,27 @@ | |||
| class ParseGithub { | |||
There was a problem hiding this comment.
Let's convert this to a regular function, Seams out of place to have a single Class with no internal functions.
src/parseGithubWebhook.js
Outdated
| return { Status: 'QA', Environment: 'Testing' }; | ||
| } | ||
| if (branch === 'staging') { | ||
| return { Status: 'Passed - QA', Environment: 'Staging' }; |
There was a problem hiding this comment.
I think this status needs to be 'CAT' not 'Passed - QA'.
src/index.js
Outdated
| commits.forEach(commit => { | ||
| console.log('commit: ', commit); | ||
| const match5OrMoreDigits = /[0-9]{5,}/; // Prevents making request for merge #'s ie "Merge pull request #12..."" | ||
| const idChecker = commit.message.match(match5OrMoreDigits); | ||
|
|
||
| if (idChecker) { | ||
| const id = idChecker[0]; |
There was a problem hiding this comment.
Can we abstract this to a function with a descriptive name? What is it actually doing, is it getTaskIDs or validateTaskIds or validateCommitMessages?
src/index.js
Outdated
| const url = `${LP_API_BASE_URL}/tasks/${id}`; | ||
| const postRequestData = { | ||
| task: { | ||
| custom_field_values: customFieldValues, | ||
| }, | ||
| }; | ||
| const postRequestConfig = { headers: { Authorization: `Bearer ${LP_API_TOKEN}` } }; | ||
|
|
||
| axios | ||
| .put(url, postRequestData, postRequestConfig) | ||
| .then(response => { | ||
| console.log(response); | ||
| }) | ||
| .catch(error => { | ||
| console.log(error); | ||
| }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This can be abstracted also, maybe create an LP API Helper that had an update task method?
| runtime: nodejs12.x | ||
| stage: ${opt:stage, 'dev'} | ||
| region: ${opt:region, 'us-west-2'} | ||
| environment: |
There was a problem hiding this comment.
The readme say to add 2 env vars to the function, but they are not defined here. Does that mean you are manually editing the deployed function in the console?
There was a problem hiding this comment.
Right now the answer is yes, but if there is a better way please let me know. I was thinking of doing param store but you also have to manually edit that on the console.
There was a problem hiding this comment.
You could create a config file to deploy those by stage. Or you can create an ssm parameter and reference that via the code.
It would be fine to create the parameter with serverless, but the value should be set manually. But that can be done in the cli also so no need to actually log into the AWS console to make changes.
Code review changes
No description provided.