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

Remove stale and unused code #771

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Remove stale and unused code #771

wants to merge 19 commits into from

Conversation

cduhn17
Copy link
Collaborator

@cduhn17 cduhn17 commented Jan 23, 2025

🗣 Description

Removed outdated and redundant code across the legacy TypeScript backend to improve clarity, maintainability, and overall project efficiency.

💭 Motivation and context

This change removes outdated TypeScript backend code to reduce technical debt and eliminate confusion from unused components. Replacing FastAPI with Django made the legacy TypeScript API obsolete, ensuring a cleaner, more maintainable codebase.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Create a release.

Copy link
Contributor

@aloftus23 aloftus23 left a comment

Choose a reason for hiding this comment

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

Just a few typos. @cduhn17 Forgot to remind you about this.

@@ -123,7 +123,7 @@ jobs:
SLS_DEBUG: '*'

- name: Deploy worker
run: npm run deploy-worker-staging
run: npm run deploy-worker.sh-staging
Copy link
Contributor

@aloftus23 aloftus23 Jan 29, 2025

Choose a reason for hiding this comment

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

Changes to this file shouldn't be necessary. Need to remove both changes.

deploy-worker is an alias defined in backend/package.json so shouldn't have .sh in it. Same with deploy-worker-integration

@@ -2,7 +2,7 @@
# Deploys worker to Terraform.
# If the worker_ecs_repository_url output from Terraform changes, you should replace "XXX.dkr.ecr.us-east-1.amazonaws.com" in this file with that URL.
# To deploy staging, run ./deploy-worker.sh.
# To deploy prod, run ./deploy-worker.sh crossfeed-prod-worker.
# To deploy prod, run ./deploy-worker.sh.sh crossfeed-prod-worker.
Copy link
Contributor

@aloftus23 aloftus23 Jan 29, 2025

Choose a reason for hiding this comment

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

Typo here. No changes needed to this file.

@@ -21,7 +32,11 @@
"url": "git+https://github.com/cisagov/crossfeed.git"
},
"scripts": {
"start": "docker compose build --parallel && docker compose up --force-recreate"
"lint": "eslint '**/*.{ts,tsx,js,jsx}'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to change this file except for your es addition. These are all the packages installed in the frontend lambda so we should be careful to add anything that isn't necessary.

This all might be fine just think we should triple-check any changes here.

api:
handler: src.xfd_django.xfd_django.asgi.handler
events:
- http:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. This file needs to be called functions.yml instead of .yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

file extension updated per this request at commit 6b598c2

@aloftus23
Copy link
Contributor

@cduhn17 Still a couple small fixes needed that I mentioned above. Otherwise looks good.

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