Skip to content

feat: insights temporal workers #325

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

epipav
Copy link
Collaborator

@epipav epipav commented Jun 3, 2025

@Copilot Copilot AI review requested due to automatic review settings June 3, 2025 15:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Temporal worker support and related services/configuration for the Insights platform, including new Docker setups, bash utilities, and workspace updates.

  • Introduce a colored logging utility script (scripts/utils) for CI/development.
  • Add Docker Compose services for the package-downloads-worker and insights-app, plus scaffolding for a local Temporal environment.
  • Update workspace and builder scripts (pnpm-workspace.yaml, init-submodules.sh, builder envs) to include new worker packages.

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
scripts/utils New bash logging helpers for colored output
scripts/services/package-downloads-worker.yaml Compose service for package-downloads Temporal worker
scripts/services/insights-app.yaml Compose service for insights-app API
scripts/services/docker/Dockerfile.package_downloads_worker.dockerignore Misplaced Dockerfile in .dockerignore
scripts/services/docker/Dockerfile.package_downloads_worker Multi-stage Dockerfile for package-downloads worker
scripts/services/docker/Dockerfile.insights_app.dockerignore Dockerignore for insights-app build context
scripts/services/docker/Dockerfile.insights_app Multi-stage Dockerfile for insights-app frontend/backend
scripts/scaffold/temporal/entrypoint.sh Entrypoint script waiting on Temporal health
scripts/scaffold/temporal/Dockerfile Dockerfile for Temporal CLI container
scripts/scaffold.yaml Scaffold Compose stack including Postgres and Temporal
scripts/builders/package-downloads-worker.env Builder environment for package-downloads-worker image
scripts/builders/insights-app.env Builder environment for insights-app image
pnpm-workspace.yaml Include new worker packages in workspace
package.json Add lint-staged config for automatic license headers
init-submodules.sh Sparse-checkout setup for crowd.dev submodule
database/flyway_migrate.sh Script to run database migrations with Flyway
database/Dockerfile.flyway Custom Flyway image for migrations
.gitmodules Add crowd.dev submodule
.env.dist.local Default local environment variables
.env.dist.composed Default composed environment overrides
Comments suppressed due to low confidence (2)

scripts/services/insights-app.yaml:9

  • The Compose file is missing a top-level 'services:' key, so the 'insights-app' service block won’t be recognized. Wrap your service definitions under 'services:'.
insights-app:

.gitmodules:1

  • The 'branch = main' line appears before the submodule block and may be ignored. It should be moved inside the '[submodule "submodules/crowd.dev"]' section or removed.
branch = main

@borfast
Copy link
Collaborator

borfast commented Jun 4, 2025

In general it looks ok, but I think we could improve the code organisation. I know you started by trying to mimic the structure we have on crowd.dev, but what we have there is a bit confusing and "entangled" with all the indirections between docker files, for example.

So my suggestions would be to:

  1. Not use the .env.dist.composed pattern and instead use the more common pattern of having a single .env.dist file for each service with default values that can be public and removing those that can´t. The user should copy the file to .env and replace the necessary values.
  2. Not use the CLI as it currently stands, because it hides away too much, makes understanding what happens harder than it needs to be, and running the underlying tools isn't that hard anyway. If we really want to have scripts abstract away the underlying tools, then we should make sure there is good documentation about the tool and perhaps also split it into multiple scripts with explicit names for their functionality.
  3. Use standard names for docker-related files, like Dockerfile and docker-compose.yaml, so that it is immediately clear what they are, and move them into a docker folder inside each service directory, instead of having them all together inside a single folder.
  4. Move the workers directory into a Temporal-dedicated folder, like services/temportal/workers.

@epipav epipav requested a review from borfast June 13, 2025 14:31
@epipav epipav force-pushed the feature/insights-temporal-workers branch from 19a8bb8 to 2275564 Compare June 16, 2025 07:45
@epipav epipav force-pushed the feature/insights-temporal-workers branch from 89c97f6 to 2d64686 Compare June 16, 2025 08:03
@github-actions github-actions bot mentioned this pull request Jun 18, 2025
emlimlf and others added 2 commits June 18, 2025 20:03
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
Co-authored-by: anilb <epipav@gmail.com>
Co-authored-by: Gašper Grom <gasper.grom@gmail.com>
Co-authored-by: joanagmaia <jmaia@contractor.linuxfoundation.org>
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