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

terarium dataservice migration changes #177

Merged
merged 30 commits into from
Feb 26, 2024
Merged

Conversation

dvince2
Copy link
Contributor

@dvince2 dvince2 commented Jan 10, 2024

This PR handles the changes needed to move to the new data service endpoints. This branch is untested currently, and contains the following changes:

  • Changing endpoints and parameters to use kebab-case
  • JSON keys are serialized / deserialized from camelCase rather than snake_case by default. Support for a "X-Enable-Snake-Case" header has been integrated which when provided will serialize / deserialize JSON keys as snake_case
  • Our backend uses OAuth2 and many endpoints require a user object for ReBAC. For simplicity we have created a shared service-user that will be injected if the correct basic auth credential is added to each request. I've introduced TDS_USER and TDS_PASSWORD env vars that will be injected in a basic auth header in each request. Please contact our team and we will provide the credential for our staging env. Please do not commit the credential to the repo unencrypted.

* Update to use new data service endpoints
* Using basic auth and snake case request headers.
* Correct casing
* Correct spacing
* Correct casing
* Updated docker compose
* Commenting out broken test
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 84.37%. Comparing base (2c32c25) to head (ac91b55).
Report is 87 commits behind head on main.

❗ Current head ac91b55 differs from pull request most recent head 8c84856. Consider uploading reports for the commit 8c84856 to get more accurate results

Files Patch % Lines
worker/operations.py 69.23% 4 Missing ⚠️
worker/utils.py 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   81.78%   84.37%   +2.59%     
==========================================
  Files           5        7       +2     
  Lines         505      845     +340     
==========================================
+ Hits          413      713     +300     
- Misses         92      132      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@YohannParis YohannParis self-assigned this Jan 31, 2024
@YohannParis YohannParis marked this pull request as ready for review February 5, 2024 13:38
@YohannParis YohannParis requested review from brandomr and fivegrant and removed request for brandomr and fivegrant February 5, 2024 19:52
Copy link
Contributor

@Sorrento110 Sorrento110 left a comment

Choose a reason for hiding this comment

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

Looks good overall, but I think some of the reporting changes to seeding may break the rest of the reporting flow.

reporting/tests/seed.py Show resolved Hide resolved
reporting/tests/seed.py Show resolved Hide resolved
@dgauldie
Copy link
Contributor

dgauldie commented Feb 5, 2024

@Sorrento110 and @fivegrant , how do we test the report generation on this branch? can one of you see if we've broken things before we merge this?

@fivegrant
Copy link
Collaborator

@fivegrant
Copy link
Collaborator

fivegrant commented Feb 5, 2024

@dgauldie just tried to run the above but the testing depends on standing up and seeding TDS in the docker compose just like simulation-integration. Report generation won't work unless HMI Server replaces TDS in the docker compose

The full flow has not been tested with this yet,
however, the test container doesn't crash at
least.
@fivegrant
Copy link
Collaborator

fivegrant commented Feb 5, 2024

@dgauldie i just pushed a fix so the tests can run. reporting was broken because stuff was moved around at project root and data service was still being referenced in a few places.

Seems to hang on seeding but the docker compose should be working for you now. It's because I have the wrong URL but you should be able to set everything up in .env to make it work

@YohannParis YohannParis merged commit dd23f09 into main Feb 26, 2024
1 check passed
@YohannParis YohannParis deleted the 2242-new-data-service branch February 26, 2024 21:38
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.

6 participants