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

Implement startProvisionJob operation #638

Merged
merged 20 commits into from
Mar 15, 2022
Merged

Conversation

juliusfitzhugh-ccpo
Copy link
Contributor

@juliusfitzhugh-ccpo juliusfitzhugh-ccpo commented Feb 28, 2022

After a reset of the repo, the previous AWS step functions (sfn) state machine is being reused for provisioning jobs from SNow.

The lambdas used in the sfn are contained in the api/provision folder.

The start-provision-job.ts lambda constructs a CspInvocation object from the ProvisionRequest and is the input into the initial state of the sfn which starts execution of the state machine. Middy is used for validation.

  • POST /provisioning-jobs is valid and starts ProvisioningStateMachine execution
  • POST /provisioning-jobs invalid inputs respond with validation errors

The provisioning-sfn-workflow.ts contains the majority of the components for setting up the sfn states and logic. This is to help reduce the logic in the atat-web-api-stack. The sample-fn.ts is merely a dummy function used as a temporary placeholder.

The api-sfn-function is used as a base function used in the sfn states, but as pointed out this is not a path we want to continue down as we have previously ran into issues.

Tests have been moved from /test and placed adjacent to the code being tested. Thejest.config.js has been updated to capture test similar to before.

Some linting and tsc errors handled in branches 677fed2 and 190b48f.

Ticket: AT-7050

Copy link
Contributor

@kylelaker-ccpo kylelaker-ccpo left a comment

Choose a reason for hiding this comment

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

More questions than anything else; just want to make sure we talk about some of this before rolling forward too much

jest.config.js Outdated Show resolved Hide resolved
lib/constructs/ApiSfnFunction.ts Outdated Show resolved Hide resolved
models/CloudServiceProviders.ts Outdated Show resolved Hide resolved
lib/atat-web-api-stack.ts Outdated Show resolved Hide resolved
lib/constructs/ApiSfnFunction.ts Outdated Show resolved Hide resolved
utils/errors.ts Outdated Show resolved Hide resolved
@juliusfitzhugh-ccpo juliusfitzhugh-ccpo force-pushed the feature/AT-7050 branch 3 times, most recently from 6a61369 to c4dc29d Compare March 9, 2022 15:08
@juliusfitzhugh-ccpo juliusfitzhugh-ccpo marked this pull request as ready for review March 9, 2022 15:18
@juliusfitzhugh-ccpo juliusfitzhugh-ccpo force-pushed the feature/AT-7050 branch 4 times, most recently from f77418d to 45a0910 Compare March 11, 2022 13:27
Add a startProvisionJob function with a unit test that takes
a provisioning request from SNOW, translate the request to
a CspInvocation request, and start execution of the
provisioning stateMachine. Code related to responses, errors,
and aws-sdk was resued.

Middy used for validation similar to previously with a
manually created schema. Also, resused code for middleware.
'esModuleInterop' was added to the tsconfig.json file to prevent
an error when running the unit test
("TypeError: (0 , core_1.default) is not a function)").

A models folder containing interfaces and schema files was added,
but this is meant mainly as a starting point, but can be changed
if the team wants to take a different approach to avoid duplication
of the specification, interface, and schema in the codebase.

Ticket: AT-7050
Add constructs to be used for building state machine workflow.
A sample func as a playholder func inside of the state machine.
Add a state machine to the atat-web-api-stack.

Attempted to reduce code used in the atat-web-api-stack for the
state machine by moving the code into constructor files.

Ticket: AT-7050
Rename files using 'kebab-case' after team discussion and
consensus. Move all test files into '/test' folder. Update
imports acccordingly.

Ticket: AT-7050
Implement feedback to refactor CSP information to ensure CSP
details are correclty linked to each CSP and simplify code.
Unable to use the refactored CloudServiceProvider class for the
'ProvisionRequest' interface which appears to be beasue the
properties are private and can not be used with the interface.

Update files using the CSP information and jest.config.

Ticket: AT-7050
Attempted to refactor the state machine components to reduce
the amount of code in the atat-web-api-stack.

Most of the code was consolidated into one file as they are all
related and used for the same state machine. The
'ProvisioningWorkflow' class was created as an approach to
simplify the tasks and funcs required for the state machine
definition. Many of the components depend on one another and
require reference to the stack scope, which makes it a bit
difficult to cleanly seperate and group things.

This is not considered to be optimal, but a start and there is
room for improvement.

Ticket: AT-7050
Add lambda function thte APIGW to invoke state machine.

Ticket: AT-7050
Add cspPortfolioIdChecker middleware to check if the portfolioId
provided by the CSP is present if the operationsType is
ADD_FUNDING_SOURCE or ADD_OPERATORS otherwise a validation error
is thrown. Add unit test.

Update package.json and package-lock.json file.

Ticket: AT-7050
Minor refactoring and removing unused code.
- remove errors and unused code
- update CSP URI
- update web-api-stack

Ticket: AT-7050
Update provisioning  workflow and add dynamic name for logs.

Reduce state machine timeout. Hardcode state code returned from
sampleFn lambda temporarily.

Update unit test with mockClient to prevent test from failing.

Ticket: AT-7050
Add environmentName for dynamic naming of logs. Update unit test
for infrastructure.

Ticket: AT-7050
Remove unnecessary console logs from middleware.

Ticket: AT-7050
Create a new file due to a constant renaming of 'provisioning'
back to 'Provisioning'. Update imports.

Ticket: AT-7050
@juliusfitzhugh-ccpo juliusfitzhugh-ccpo force-pushed the feature/AT-7050 branch 3 times, most recently from 107349f to deec69e Compare March 11, 2022 16:02
Refactor to use HttpMethods in lib folder. Add CspInvocation
interface.

Update payload property in schema to limit properties in the
ProvisionRequest.

Ticket: AT-7050
Relocate tests closer to the code being tested and update imports.

Ticket: AT-7050
Fix 'aws-lambda' linting error.
- typescript-eslint/typescript-eslint#1624

Disable  max-len linting error for response.ts.

Add @types/aws-sdk to fix tsc compling error related to '@aws-sdk/client-s3`.

Update tsconfig.json with 'dom' to fix ts compling error related to
'Cannot find name 'ReadableStream''

Ticket: AT-7050
When mockClient is imported it causes errors
 1) Cannot find name 'ReadableStream'. -> add 'dom' to tsconfig
 2) on github checks fails with
   Error: node_modules/aws-sdk-client-mock/dist/types/libStorage.d.ts(8,95): error TS2307:
   Cannot find module '@aws-sdk/client-s3' or its corresponding type declarations.

When removing the mockClient import and skipping tests under
"Successful provisioning operations", the checks pass.

Testing to see if adding @aws-sdk/client-s3 as dev helps.
- appears to have worked after installing this, but not sure
- removing to try and see if the error comes back
- error does return, so adding @aws-sdk/client-s3 back as dep

Ticket: AT-7050
Remove comments and address sonarclod code smells

Ticket: AT-7050
Add required properties to scheam. Update unit tests.

Ticket: AT-7050
Copy link
Contributor

@jeffsegal-ccpo jeffsegal-ccpo left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few small items to consider.

api/provision/start-provision-job.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.test.ts Outdated Show resolved Hide resolved
lib/atat-web-api-stack.ts Show resolved Hide resolved
models/provisioning-jobs.ts Show resolved Hide resolved
api/provision/sample-fn.ts Outdated Show resolved Hide resolved
api/provision/sample-fn.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.ts Show resolved Hide resolved
models/cloud-service-providers.ts Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
utils/middleware/check-csp-portfolio-id.ts Outdated Show resolved Hide resolved
utils/middleware/error-handling-middleware.ts Outdated Show resolved Hide resolved
utils/middleware/error-handling-middleware.ts Outdated Show resolved Hide resolved
utils/errors.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonburkert-ccpo jasonburkert-ccpo left a comment

Choose a reason for hiding this comment

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

Nice work. I left a few minor comments.

utils/cors-config.ts Outdated Show resolved Hide resolved
lib/atat-web-api-stack.ts Outdated Show resolved Hide resolved
lib/atat-web-api-stack.ts Outdated Show resolved Hide resolved
lib/constructs/provisioning-sfn-workflow.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.ts Outdated Show resolved Hide resolved
api/provision/start-provision-job.test.ts Outdated Show resolved Hide resolved
@juliusfitzhugh-ccpo juliusfitzhugh-ccpo force-pushed the feature/AT-7050 branch 2 times, most recently from 28e967a to fe51d0d Compare March 15, 2022 13:32
- Remove CORS related code
- Minor refactoring
- Updating comments

Removing 'dom' from tsconfig.json produces error during tsc build
during github action Run Tests.
- required to workaround aws/aws-sdk-js-v3#2896

Ticket: AT-7050
Copy link
Contributor

@jasonburkert-ccpo jasonburkert-ccpo left a comment

Choose a reason for hiding this comment

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

Three small things for follow-up

lib/constructs/provisioning-sfn-workflow.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
api/provision/start-provision-job.test.ts Outdated Show resolved Hide resolved
- remove @middy/http-cors from package.json
- remove symbol from comment
- update sourceIp

Ticket: AT-7050
@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.4% 90.4% Coverage
0.0% 0.0% Duplication

@jasonburkert-ccpo jasonburkert-ccpo merged commit 9945cd4 into develop Mar 15, 2022
@jasonburkert-ccpo jasonburkert-ccpo deleted the feature/AT-7050 branch March 15, 2022 15:17
juliusfitzhugh-ccpo added a commit that referenced this pull request Mar 15, 2022
Refactor based on feedback from
- #638 (comment)

Ticket: AT-7055
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants