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

refactor: remove unused and outdated files #1624

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Junjiequan
Copy link
Contributor

@Junjiequan Junjiequan commented Oct 22, 2024

Description

This PR aims to cleanup outdated CI/E2E configurations.

Main fixes:

  • Moved all the oudated facility specific config files to the to_be_deleted folder.
  • Removed the pipeline triggers for ESS and MAX dev environments on master branch pushes, as both pipelines have been inactive for a significant time.
  • Removed the ESS-specific scichat-loopback microservice from the e2e test Docker Compose file
  • Removed workflow that builds & push docker images based on manual release.
  • Reduced the number of Cypress configuration files to a single cypress.config.ts.
  • Fixed Jasmine type assertion falsy errors (e.g., Property 'toBeFalsy' does not exist on type 'Assertion').
  • Fixed Cypress defineConfig type issues (e.g.,Object literal may only specify known properties, but 'lbBaseUrl' does not exist in type 'EndToEndConfigOptions'.).

Motivation

The CI and testing setup has accumulated several unused and unmaintained configuration files, leading to increased difficulty in maintaining and debugging the system

Fixes:

Please provide a list of the fixes implemented in this PR

  • Items added

Changes:

Please provide a list of the changes implemented by this PR

  • changes made

Tests included

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)

Documentation

  • swagger documentation updated [required]
  • official documentation updated [nice-to-have]

official documentation info

If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included

Backend version

  • Does it require a specific version of the backend
  • which version of the backend is required:

Summary by Sourcery

Refactor the project by removing unused and outdated files, updating docker-compose configurations, and optimizing CI workflows. Update service images and configurations to improve compatibility and performance. Remove unnecessary pipeline triggers and enhance test workflow settings.

Enhancements:

  • Update Traefik image to version 2.10 in the docker-compose configuration.
  • Switch MongoDB image to 'mongo:latest' due to ARM64 support issues with Bitnami MongoDB.
  • Update SciChat Loopback and backend service image tags to 'latest' in the docker-compose configuration.
  • Add health check for the backend service in the docker-compose configuration.
  • Remove unused and outdated files from the CI and deployment configurations.

CI:

  • Remove ESS and MAXIV pipeline triggers from the deploy workflow.
  • Optimize memory and file handling settings for Elasticsearch in the test workflow.
  • Update Cypress configuration file paths and base URLs in the test workflow.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Junjiequan - I've reviewed your changes - here's some feedback:

Overall Comments:

  • The removal of the ESS and MAXIV pipeline triggers in the deploy.yml workflow is a significant change. Please document the reasons for their removal and provide information on any replacement processes for these deployments.
  • Consider reviewing the startup dependencies for all services in the docker-compose file. While you've added a health check for the backend service, ensure that all services start up in the correct order to prevent potential race conditions.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Just one question.
The rest looks fine to me.

"-g",
"daemon off;"
]
dockerfile: Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that we do not need to wait for the service to come on line?

Copy link
Contributor

@minottic minottic left a comment

Choose a reason for hiding this comment

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

as this PR seems to do some cleanup, I would suggest to have a look at this, at least for deploy.yml, maybe as food for thoughts for another PR (or at leat an issue). In this way we could reuse the deploy.yml workflow in all repos and import from a central place

depends_on:
mongodb:
condition: service_healthy

backend:
image: ghcr.io/scicatproject/backend-next:latest
command: sh -c "node dist/main"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the command is part of the image already and could be removed

@@ -1,14 +1,16 @@
version: "3.4"
services:
reverse-proxy:
image: traefik:2.5
image: traefik:2.10
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what we need traefik for

"-g",
"daemon off;"
]
dockerfile: Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line could be removed as Dockerfile should be the defautl

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.

3 participants