-
Notifications
You must be signed in to change notification settings - Fork 28
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
uncomment and use get_asn #1320
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1320 +/- ##
==========================================
- Coverage 78.69% 78.56% -0.14%
==========================================
Files 117 117
Lines 7844 7860 +16
==========================================
+ Hits 6173 6175 +2
- Misses 1671 1685 +14
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
@ddavis-stsci I requested your review since using |
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 to me, @braingram! Thanks!
18d268f
to
70eb70e
Compare
@mairanteodoro Thanks for taking a look. I'm seeing this PR as not approved. Is that intentional? If so, is there an issue to address or another person to approve it. Thanks again. |
I was just deferring the approval to @ddavis-stsci. I will aprove it now. |
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.
Thanks, @braingram!
Thanks! @ddavis-stsci Does this look good to you? It will be useful for #1348 I'm happy to either wait until after #1348 is merged and then update this PR or merge this PR so you can update #1348 |
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.
LGTM
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 to me.
Thanks! I'm re-running the regtests now (same link as above). EDIT: this needs a new run (and new link) due to the patch table environment variable changes |
Regtests all passed. |
Fixes #1318
This PR uncomments
get_asn
and uses it in the 2 regtests I found that pull associations from artifactory.This allows the regtests to be simplified by not requiring them to:
get_asn
automatically sets thertdata.input
attribute so the test does not need to set thisRegression tests running:
https://github.com/spacetelescope/RegressionTests/actions/runs/10267768330
Checklist
CHANGES.rst
under the corresponding subsection