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

feat: migrate stfc users service to rest #897

Merged
merged 69 commits into from
Jan 31, 2025

Conversation

William-Edwards-STFC
Copy link
Contributor

@William-Edwards-STFC William-Edwards-STFC commented Dec 17, 2024

Closes UserOfficeProject/issue-tracker#1060

Description

This PR introduces REST changes to the application, replacing SOAP with REST in STFC UserOfficeWebService, and updating relevant configurations and dependencies.

Motivation and Context

The change is required to migrate from the SOAP-based STFC UserOfficeWebService to a REST-based service. This will provide a more modern, efficient, and easy-to-maintain approach to data communication, ultimately improving the performance and reliability of the application.

Please find the mock server pr below.

UserOfficeProject/mock-uows#18

Please find the docker orchestration pr below.

https://github.com/isisbusapps/docker-orchestration/pull/687

Changes

  1. Updated .prettierignore, .vscode/launch.json, README.md, apps/backend/.eslintignore, apps/backend/.gitignore, and apps/backend/docker-compose-stfc.e2e.yml files to reflect the migration from SOAP to REST.
  2. Introduced the openapi-typescript-codegen package to generate TypeScript/JavaScript client APIs from OpenAPI Specification (OAS).
  3. Removed the SOAP client for STFC UserOfficeWebService and replaced it with an auto-generated REST client.

How Has This Been Tested?

Fixes Jira Issue

https://jira.esss.lu.se/browse/

Depends On

Tests included/Docs Updated?

  • I have added tests to cover my changes.
  • All relevant doc has been updated

@William-Edwards-STFC William-Edwards-STFC changed the title Rest changes feat: migrate stfc users service to rest Dec 17, 2024
@William-Edwards-STFC William-Edwards-STFC marked this pull request as ready for review January 14, 2025 11:33
@William-Edwards-STFC William-Edwards-STFC requested a review from a team as a code owner January 14, 2025 11:33
@William-Edwards-STFC William-Edwards-STFC removed the request for review from a team January 14, 2025 11:33
Copy link
Contributor

@mutambaraf mutambaraf 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 to me but have we resolved adding openapi.yaml to this repo

@ACLay
Copy link
Contributor

ACLay commented Jan 28, 2025

I don't see how we could avoid including an api spec or client. We need a client for the code to compile, so we either include that as client code, or the service definition to generate one at build time. I guess it might be possible to have the client as a separate npm package, but the extra infrastructure for that seems excessive compared to just adding an npm command and yaml file here. And it's not like having a client/spec here is new, the old UOWSSoapInterface.ts is generated source code for the SOAP client.

@TCMeldrum
Copy link
Contributor

TCMeldrum commented Jan 28, 2025

I agree with alex, I don't see a way around adding the open-api file or the compiled code to this repo without massive hassle to both us and other collaborators. I think I prefer adding the open-api.yaml rather than a folder of regenerated code.

@mutambaraf
Copy link
Contributor

We have more like a month to come up with a package which l think might be a good solution here.

@ACLay
Copy link
Contributor

ACLay commented Jan 28, 2025

Sure we've got a month, but what is the problem that having a separate package would address? It'd be adding extra steps to our update process if we need a new client to pull in UOWS updates, and giving us more infrastructure to maintain, so there'd have to be some benefit for that to be worth it, but I don't see what that would be.

Copy link
Contributor

@jekabs-karklins jekabs-karklins left a comment

Choose a reason for hiding this comment

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

Approved! But please don't forget to review the two comments I've added, and adjust as you see fit.

@William-Edwards-STFC William-Edwards-STFC dismissed ACLay’s stale review January 31, 2025 13:24

I have made the requested changes

@William-Edwards-STFC William-Edwards-STFC merged commit c33ee3d into develop Jan 31, 2025
20 checks passed
@William-Edwards-STFC William-Edwards-STFC deleted the migrate-rest-new branch January 31, 2025 13:24
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.

Migrate to the new users REST service
5 participants