-
Notifications
You must be signed in to change notification settings - Fork 0
In 1052 add guardrails final runs #83
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
Conversation
f948ed4 to
fdc5310
Compare
Pull Request Test Coverage Report for Build 10792977483Details
💛 - Coveralls |
57f7fae to
f5cab96
Compare
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.
Overall, looking good. I particularly appreciate the detailed PR notes and examples.
Left a couple questions that likely could be resolved later, or not at all.
I did leave one question which prompted this "Request Changes" review, as it seems like it could be a bug? Unless I'm not understanding it?
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.
Good changes and the context in the PR description is greatly appreciated as always! I'm also curious about the issues @ghukill raised so just commenting for now
8e37437 to
1ca21d1
Compare
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.
Looks good!
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.
Looking great to me.
I left a comment around the get_task_status_and_logs method, as I did find it a little hard to follow, but I'm okay approving without changes there; it's mostly just stylistic / readability, and could be a personal preference.
1ca21d1 to
c194393
Compare
Why these changes are being introduced: * Add guardrails to prevent accidental executions of 'final' runs by adding a confirmation page to prompt user for input (selecting a button) before actual execution of a 'final' run. How this addresses that need: * Update Flask app to support additional "confirmation" route * Protect 'execute' route by verifying referrer for 'final' runs Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1052
Why these changes are being introduced: * The deprecated methods previously caused the request to the status 'data' route for retrieving CloudWatch logs progressively got slower as the number of log streams increased. By removing these methods, the client relies on boto3's error handling to efficiently detect invalid task IDs. How this addresses that need: * Update get_log_events to rely on ResourceNotFoundException * Deprecate unneeded methods Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1057
c194393 to
0ca0fc9
Compare
Purpose and background context
Add guardrails to prevent (or at least minimize) accidental executions of "final" runs of Alma SAP Invoices processing. This is achieved by creating a confirmation page that the Flask application directs users to when selecting to
Execute a final run(see screenshots below). This PR also includes the following updates:webapp.config.Configto construct theALMA_SAP_INVOICES_NETWORK_CONFIGvariable from its componentsALMA_SAP_INVOICES_ECS_GROUPSandALMA_SAP_INVOICES_ECS_SUBNETS.@propertymethods for environment variables. Remember that the original purpose of these now-deprecated methods was to load config vars into Flask'sapp.configmodel. Since we are not actually doing this, I opted to returnwebapp.config.Configto our standard format.It's also worth noting the following discovery: "Expired" CloudWatch logs are still findable by
boto3.CloudWatchLogs.Client.get_log_events; the client will return a successful200response but an empty list. This necessitated a few updates to the "status" page of the SAP Invoices web app. The "status" page displays the following results:When page first loads.

Valid task ID and CloudWatch logs are not expired

Valid task ID and CloudWatch logs are expired

Invalid task ID (leads to error page)

How can a reviewer manually see the effects of these changes?
It is recommended that the changes be reviewed by commit! There aren't a lot of lines changed from these updates, but a lot of files did change.
Addition of confirmation page
Do one of the following:
Review screenshot(s):

Visit https://sapinvoices.dev1.mitlibrary.net/.
Process invoicesExecute a final run.Improved runtime

This 1st screenshot shows the runtime of the request for retrieving logs from CloudWatch logs for an old task (i.e., not within the last hour), which is done via the data route before the changes:
This 2nd screenshot shows the runtime of the request ... for the same task ID after the changes:

Note that in the 2nd screenshot, the time is not increasing with each call to the data route.
Env variables
Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)