Skip to content

Conversation

@MrCreosote
Copy link
Member

@MrCreosote MrCreosote commented Sep 12, 2025

  • Switches USE_SHIFTER -> USE_EXTERNAL_URLS to better describe what the env var does
  • Moves the url mangling logic into config, other than the separate ee2 config
  • Swtiches an error() log to a log() log as it's not actually a error, it happens all the time and is normal.

Will need to be tested in situ at NERSC.

* Switches USE_SHIFTER -> USE_EXTERNAL_URLS to better describe what the
env var does
* Swtiches an error() log to a log() log as it's not actually a error,
it happens all the time and is normal.
@codecov
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.84%. Comparing base (5170db0) to head (676da48).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
JobRunner/JobRunner.py 42.85% 4 Missing ⚠️
JobRunner/config.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
+ Coverage   80.71%   80.84%   +0.13%     
==========================================
  Files          13       13              
  Lines        1177     1180       +3     
==========================================
+ Hits          950      954       +4     
+ Misses        227      226       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

... other than the ee2 config

Will need to be tested in situ
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors URL management for the NERSC environment by switching from the USE_SHIFTER environment variable to USE_EXTERNAL_URLS and consolidating URL replacement logic. It also changes an error log to an informational log for a common non-error condition.

  • Renamed environment variable USE_SHIFTER to USE_EXTERNAL_URLS for clearer intent
  • Consolidated URL replacement logic into the Config class
  • Changed Docker image not found error logging from error to info level

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
scripts/jobrunner.py Removed URL replacement logic, moved to Config class
JobRunner/config.py Added URL constants and URL replacement logic in Config constructor
JobRunner/JobRunner.py Updated to use consolidated config and renamed variables for clarity
JobRunner/DockerRunner.py Changed Docker image not found logging from error to info level

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 38 to 39
PROD_INTERNAL_URL_BASE = "https://services.kbase.us"
PROD_EXTERNAL_URL_BASE = "https://kbase.us"
Copy link

Copilot AI Sep 15, 2025

Choose a reason for hiding this comment

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

Extra space before the equals sign in class constants. Should be single space for consistency with Python style guidelines.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

ee2_config = self.ee2.list_config()
except Exception as e:
self.logger.error("Failed to config . Exiting.")
self.logger.error("Failed to get config . Exiting.")
Copy link

Copilot AI Sep 15, 2025

Choose a reason for hiding this comment

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

Extra space before the period in the error message. Should be 'Failed to get config. Exiting.' for consistency.

Suggested change
self.logger.error("Failed to get config . Exiting.")
self.logger.error("Failed to get config. Exiting.")

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@bio-boris bio-boris requested a review from Copilot September 15, 2025 21:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@MrCreosote MrCreosote merged commit 78f0fb6 into main Sep 16, 2025
11 of 12 checks passed
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.

3 participants