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

sync/allow-latest #1088

Merged

Conversation

jdavies-st
Copy link
Contributor

@jdavies-st jdavies-st commented Oct 10, 2024

Resolves #1087

Allow "latest" as a CRDS file state in crds sync so that it doesn't issue a warning message for all the files it finds in this state.

There's no test for crds sync with JWST. Only HST. And the testing infrastructure here is so specialized, I'm not sure how to test this. But some pseudocode would be:

def test_sync_jwst_file_state():
    with pytest.warns() as record:
        errors = SyncScript("crds.sync some low-resource-command that reproduces warning")()
    assert errors is None
    assert len(record) is 0, record.pop().message

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

@jdavies-st jdavies-st requested a review from a team as a code owner October 10, 2024 09:48
@stscieisenhamer stscieisenhamer self-assigned this Oct 10, 2024
Copy link
Collaborator

@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

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

LGTM. We'll figure out some test for it. In the meantime, would you mind either 1) adding a news fragment to the jwst section or "allow edits from maintainers" on the PR.

If neither, that's ok, just let us know. Thanks!

@alphasentaurii alphasentaurii changed the title Allow 'latest' as a CRDS file state for crds sync sync/allow-latest Oct 10, 2024
Copy link
Collaborator

@alphasentaurii alphasentaurii left a comment

Choose a reason for hiding this comment

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

Added the news fragment and made an additional adjustment related to this fix. Also modified the CI context retrieval for JWST to include the state arg "latest". Will need to do the same for other observatories once 12.x is deployed there. There are a number of tests that will need to be added and updated following those deployments so that will be done in a separate PR.

@alphasentaurii alphasentaurii merged commit 64a5cf9 into spacetelescope:master Oct 11, 2024
3 of 5 checks passed
@jdavies-st jdavies-st deleted the cron-sync-allow-state-latest branch October 11, 2024 23:00
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.

Slew of "has an unusual CRDS file state 'latest'" WARNINGS with cron_sync
3 participants