Skip to content

Conversation

@matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Nov 17, 2025

Description

Move ufo-data validate obs file logic to ufo.

Issue(s) addressed

Resolves #514.

This may not be the best solution for #514, but is certainly the easiest to implement, as we are only moving the validation tests from ufo-data to ufo. With the test in ufo, it will now validate test data obtained from sources other than ufo-data.

Dependencies

  • JCSDA-internal/ufo#3897

build-group=JCSDA-internal/ufo#3897

Impact

Validation of obs file moved to ufo.

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation -na-
  • I have run the unit tests before creating the PR

@srherbener
Copy link
Collaborator

I think the CI test failure is due to the need to disable the CI testing for this repo. Removing the validation tests brings the total number of tests to zero and the CI test failure is that no tests are found. The other test data repositories (<repo>-data) do not have a CI subdirectory like this one so I'm guessing that removing the CI subdirectory is part of the actions needed to disable the CI testing. @eap can you help us with this - is it as simple as removing the CI subdirectory or is there more to it? Thanks!

Copy link
Collaborator

@srherbener srherbener left a comment

Choose a reason for hiding this comment

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

As I mentioned in the companion PR (jcsda-internal/ufo/pull/3897) I like this solution, so I'm going to approve. However, we need to address the CI testing failure (which I'm hoping is simply removing the CI subdirectory) before merging.

@matthewrmshin matthewrmshin marked this pull request as ready for review November 18, 2025 08:49
@matthewrmshin matthewrmshin requested a review from eap November 18, 2025 16:27
@BenjaminRuston BenjaminRuston added coordinate merge Ready for merge but needs to be coordinated with other repos OBS OBS processing, UFO labels Nov 18, 2025
@rajichidamb
Copy link
Contributor

I think the CI test failure is due to the need to disable the CI testing for this repo. Removing the validation tests brings the total number of tests to zero and the CI test failure is that no tests are found. The other test data repositories (<repo>-data) do not have a CI subdirectory like this one so I'm guessing that removing the CI subdirectory is part of the actions needed to disable the CI testing. @eap can you help us with this - is it as simple as removing the CI subdirectory or is there more to it? Thanks!

Talking to @eap, for now we can disable the workflow in Github UI, it is temporary fix for now. Once this PR is merged there will be no ctest to be run in CI, by disabling the workflow CI will not run. Since this is temporary later we need to discuss about removing CI from the Github or continue with disabled workflow until we are sure.

image

Copy link
Collaborator

@huishao-r huishao-r left a comment

Choose a reason for hiding this comment

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

CI failure is expected due to the removal

@huishao-r huishao-r merged commit 525ec40 into develop Nov 19, 2025
2 of 3 checks passed
@huishao-r huishao-r deleted the feature/ufo-data-validate branch November 19, 2025 21:24
@huishao-r
Copy link
Collaborator

@mikecooke77 We noticed this affects MetOffice ufo-data CI tests (shown in UFO PRs). Due to this merge, the ufo-data CI test from our side is turned off for now. We are going to take a final review to see whether CI tests are necessary anymore. Meanwhile, please check whether any action should taken from your side related to this PR. Ping @rajichidamb as well in case she needs to chime in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coordinate merge Ready for merge but needs to be coordinated with other repos OBS OBS processing, UFO

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[New issue] CMake issue due to ioda dependency

6 participants