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

Alma job names handling #146

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Alma job names handling #146

merged 3 commits into from
Aug 29, 2023

Conversation

adamshire123
Copy link
Contributor

What does this PR do?

Supports storing job names in SSM and including the AWS environment as part of the job name

Includes new or updated dependencies?

YES

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/ENSY-178

Developer

  • All new ENV is documented in README
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Why these changes are being introduced:
We changed some configurations in AWS so that the job names are all
stored in SSM. Because of this we can include the AWS environment in
the job name, which should be the complete job name for POD and Bursar
jobs, TIMDEX is an exception. The TIMDEX job name should include the
AWS environment, but is still technically a prefix because TIMDEX job
names end with the type (full or daily), but we don't want to have two
separate environment variables for each of those job types.

How this addresses that need:
* Updated readme
* Changed the name of the bursar job name env variable
* Reordered the check for matching environments to come first
* updated the tests to use the new variable name
* updated the tests to inclue the environment name in test environment
variables
Copy link
Contributor

@ehanson8 ehanson8 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 to me, I like the log message updates. Just 2 typos

@@ -29,6 +29,10 @@
logger.info("No Sentry DSN found, exceptions will not be sent to Sentry")


class JobTypeError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

Typos for unknown and received

tests/conftest.py Show resolved Hide resolved
Copy link
Contributor

@ghukill ghukill 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 to me.

@@ -29,6 +29,10 @@
logger.info("No Sentry DSN found, exceptions will not be sent to Sentry")


class JobTypeError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this explicit exception type here.

@github-actions
Copy link

Pull Request Test Coverage Report for Build 6015629858

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 5906345756: 0.0%
Covered Lines: 142
Relevant Lines: 142

💛 - Coveralls

@adamshire123 adamshire123 merged commit d63855c into main Aug 29, 2023
3 checks passed
@adamshire123 adamshire123 deleted the alma-job-names-handling branch August 29, 2023 17:52
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.

None yet

3 participants