Skip to content

Stabilize CI runtime dependencies#298

Open
krowvin wants to merge 1 commit into
mainfrom
fix/ci-runtime-and-cda-auth
Open

Stabilize CI runtime dependencies#298
krowvin wants to merge 1 commit into
mainfrom
fix/ci-runtime-and-cda-auth

Conversation

@krowvin

@krowvin krowvin commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Pin the unit-test workflow to Python 3.12 instead of the floating 3.x selector.
  • Include the Python version in the virtualenv cache key.
  • Pin the local CDA image default to a dated development image instead of the floating latest tag.
  • Align the local CDA compose stack on the q0 web user used by current CDA local setup and the seeded test API keys.
  • Enable CDA legacy plaintext API-key support for this local test stack through a mounted feature file.
  • Update the timeseries group delete integration test to use cascade delete instead of PATCHing an empty assigned-timeseries list.

Root cause

The failing unit-test job was selecting Python 3.14.6, where the current locked pandas/numpy stack segfaults in the mock ratings test. Unrelated PRs started failing before the LRTS branch, and this PR confirmed the Python 3.12 pin makes those jobs green.

The integration job was failing with 401 API-key errors from the local CDA stack. Current CDA only accepts the plaintext keys seeded by this repo's users.sql when AUTH_RE_ENABLE_NON_HASH_KEY_SUPPORT is enabled. After that was fixed, the remaining failure was a CDA 500 from PATCHing an empty assigned-timeseries list; the test now uses the delete endpoint's cascade behavior.

Validation

  • docker compose config --quiet
  • PR Actions: unit run-tests jobs passed after pinning Python 3.12; integration progressed from 401s to 69/70 passing before the test cleanup.
  • Local pytest verification was attempted, but this machine only has Python 3.14 installed; poetry install fails building the locked NumPy version without a compiler. Full verification is through Actions.

@krowvin krowvin force-pushed the fix/ci-runtime-and-cda-auth branch 2 times, most recently from ac1d854 to da156ba Compare July 7, 2026 15:52
@krowvin krowvin force-pushed the fix/ci-runtime-and-cda-auth branch from da156ba to df6e646 Compare July 7, 2026 16:01
@sonarqubecloud

sonarqubecloud Bot commented Jul 7, 2026

Copy link
Copy Markdown

@krowvin krowvin marked this pull request as ready for review July 7, 2026 17:05

@krowvin krowvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Few thoughts I had while making this PR

Comment thread docker-compose.yml
traefik:
condition: service_healthy
image: ${CWMS_DATA_API_IMAGE:-ghcr.io/usace/cwms-data-api:latest}
image: ${CWMS_DATA_API_IMAGE:-ghcr.io/usace/cwms-data-api:develop-2026.05.30}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I still am not 100% sure on if we want to lock the versions in.

If we add a new parameter, for example, we would then need to make sure this gets updated to match.

And it maybe confusing to determine that was the cause/need from the logs alone.

cwmsjs is getting around this by being generated as CDA updates

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

no and definitely should not be that version. that is behind what is in production.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if the latest image is getting updated frequent enough? @MikeNeilson

Should we target the current production version 2026.05.12-f? If a develop version is targeted for the tests then things might pass that are not on prod yet.

I was trying to target something around this.

@@ -0,0 +1,3 @@
AUTH_RE_ENABLE_NON_HASH_KEY_SUPPORT=true

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This should make it so we don't need to hash the keys locally and our old keys keep working

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is easy to add the hashed keys I will set that up.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Want to PR off this or make a separate PR for that?

If you are busy we could merge this in now so the old keys keep working then your PR can use the new ones?

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.

2 participants