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

add second required argument to jwst get_default_context curl command #1086

Closed
wants to merge 1 commit into from

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Oct 9, 2024

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
news fragment change types...
  • changes/<PR#>.hst.rst: HST reference files
  • changes/<PR#>.jwst.rst: JWST reference files
  • changes/<PR#>.roman.rst: Roman reference files
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.testing.rst: change to tests or test automation
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@braingram braingram marked this pull request as ready for review October 9, 2024 21:22
@braingram braingram requested a review from a team as a code owner October 9, 2024 21:22
@braingram
Copy link
Contributor Author

The workflow is now working again as seen by a context being found:
https://github.com/spacetelescope/crds/actions/runs/11263290153/job/31320834838?pr=1086#step:5:5
comparing that to the latest run on jwst main:
https://github.com/spacetelescope/jwst/actions/runs/11261422647/job/31314829770#step:5:5

@braingram
Copy link
Contributor Author

I can't request reviewers or add labels. I'd say this is a "no changelog" change and perhaps @alphasentaurii would give this a review.

One open question is if similar server-side API changes are planned for hst and roman. If so, the workflow will need to be updated for those calls (when the servers are updated).

@braingram
Copy link
Contributor Author

braingram commented Oct 9, 2024

The download and cache workflow is failing:
https://github.com/spacetelescope/crds/actions/runs/11263290153/job/31320840346?pr=1086#step:11:4492
Maybe it's related to the change in this PR but I don't at the moment see the link.

@alphasentaurii
Copy link
Collaborator

alphasentaurii commented Oct 11, 2024

Sorry just seeing this now - I implemented this exact change for JWST in another PR (after looking at your suggestion on Slack re: stdatamodels) that was merged last night to resolve related sync errors (see release 12.0.3). Rather than close this PR, if you want you can rebase from master and add the null arg to HST and Roman as well. In the mean time, I'll deploy the DRC-style code changes to both of those observatories and then we can merge this in.

@alphasentaurii
Copy link
Collaborator

The download and cache workflow is failing: https://github.com/spacetelescope/crds/actions/runs/11263290153/job/31320840346?pr=1086#step:11:4492 Maybe it's related to the change in this PR but I don't at the moment see the link.

When I made this change as part of #1088 the CI didn't succeed until after I merged

@zacharyburnett
Copy link
Collaborator

should we add 'latest' to the other two curl commands for hst and roman as well?

@braingram
Copy link
Contributor Author

Thanks for taking a look. I'm going to close this PR. jwst and soon romancal and gwcs have modified versions of this workflow. jwst main is currently using null as the second argument (whereas it's using latest for the workflow in crds main). It might make sense to just ditch the curl commands and use the crds client although a decision about latest vs null will likely have to be answered for that change as well.

@braingram braingram closed this Oct 21, 2024
@braingram braingram deleted the contexts_workflow branch October 21, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants