Issue/112#180
Conversation
jackiryan
left a comment
jackiryan
left a comment
There was a problem hiding this comment.
Overall this is a reasonable change but the biggest thing is that the lambda name for the token dispenser is hardcoded and there is an account number hidden in a cassette. Also consider moving the two new cassettes into the tests directory.
|
|
||
| # Invoke the Lambda synchronously and get the token | ||
| response = lambda_client.invoke( | ||
| FunctionName='sndbx-launchpad_token_dispenser', |
There was a problem hiding this comment.
Is this always the name for the token dispenser lambda? The sndbx designation seems suspicious. Should this be configured in terraform?
There was a problem hiding this comment.
Is there a reason why the two cassettes in the cassettes directory are not in tests/cassettes? You should move them into the tests directory
| body: | ||
| string: '<?xml version="1.0" encoding="UTF-8"?> | ||
|
|
||
| <Error><Code>AccessDenied</Code><Message>User: arn:aws:iam::536711851782:user/NGAPShApplicationDeveloper-bcwong1-580 |
There was a problem hiding this comment.
Account number and ARN for a user are exposed, this should be removed from the cassette file.
There was a problem hiding this comment.
This is good to see the cassette can be a lot simpler. I verified the tests pass on my end so I must have been overspecifying information.
| mock_boto.return_value = mock_lambda | ||
|
|
||
| result = get_harmony_client('PROD') | ||
| # 👇 double-encoded JSON (required by function) |
There was a problem hiding this comment.
Remove emoji in comment, this is just a style thing.
There was a problem hiding this comment.
Sonar Cube is dinging you for only having 68% coverage on the lines you added to utils.py, the minimum bar it sets is >=80%. You could add more unit tests but it's not required, we can merge with SonarCube "failing"
|
One more thing @DavidVWoodSr this code fails in SIT with the following error at the Submit Harmony Job step: Sounds like you can't pass the token as just a string, it has to be like |
3 participants
Fixes Github Issue: #140
Description
Summarize the ticket here
Overview of work done
Summarize the work you did
Overview of verification done
Summarize the testing and verification you've done. This includes unit tests or testing with specific data
Overview of integration done
Explain how this change was integration tested. Provide screenshots or logs if appropriate.
PR checklist:
See Pull Request Review Checklist for pointers on reviewing this pull request