Skip to content

refactor: Move ContainerExecutionStatus to enums module#202

Closed
morgan-wowk wants to merge 1 commit intomasterfrom
enums-refactor
Closed

refactor: Move ContainerExecutionStatus to enums module#202
morgan-wowk wants to merge 1 commit intomasterfrom
enums-refactor

Conversation

@morgan-wowk
Copy link
Copy Markdown
Collaborator

Breaks the circular import that prevented event_listeners.py from typing
StatusTransitionEvent fields as ContainerExecutionStatus. All consumers
updated to import from the new enums module.

Copy link
Copy Markdown
Collaborator Author

morgan-wowk commented Apr 8, 2026

@morgan-wowk morgan-wowk marked this pull request as ready for review April 9, 2026 00:35
@morgan-wowk morgan-wowk requested a review from Ark-kun as a code owner April 9, 2026 00:35
Comment thread cloud_pipelines_backend/api_router.py Outdated

from . import api_server_sql
from . import backend_types_sql
from . import enums
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Would be better to be a bit descriptive for the name here like

  • container_statuses
  • models
  • statuses

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do you suggest a folder instead?

enums
\_container_statuses.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not a folder, just renaming the file. It's just that enums and enum (python's module) are so close, it's easy for my to confuse the two.

Mainly, it's probably good practice to describe the functionality of the file, not the technology. For example, if i named a file classes.py or functions.py, it'll be ambiguous.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Changed

Breaks the circular import that prevented event_listeners.py from typing
StatusTransitionEvent fields as ContainerExecutionStatus. All consumers
updated to import from the new enums module.
Copy link
Copy Markdown
Contributor

Ark-kun commented Apr 9, 2026

>Breaks the circular import that prevented event_listeners.py from typing
StatusTransitionEvent fields as ContainerExecutionStatus.

Hmm. Let's solve this.

  1. There should not be circular imports in this area
  2. Circular imports are possible in Python

@Ark-kun Ark-kun self-assigned this Apr 9, 2026
Copy link
Copy Markdown
Contributor

Ark-kun commented Apr 9, 2026

From 1-1 discussion:
Let's have a simple solution like this:

  • backend types remain unchanged
  • orchestrator listens to status change events and adds them to history. Maybe, the status change handler will need to add a ExecutionNode._container_execution_status_changed = True mark. Telemetry is not involved here.
  • telemetry listens to pre-commit and post-commit events. On pre-commit, it stashes the status change info it needs to send. On post-commit it sends out that info. Alternatively, we could just send the info in pre-commit hook. The chances of that transaction commit failing are close to 0.

Copy link
Copy Markdown
Collaborator Author

From 1-1 discussion:
Let's have a simple solution like this:

  • backend types remain unchanged
  • orchestrator listens to status change events and adds them to history. Maybe, the status change handler will need to add a ExecutionNode._container_execution_status_changed = True mark. Telemetry is not involved here.
  • telemetry listens to pre-commit and post-commit events. On pre-commit, it stashes the status change info it needs to send. On post-commit it sends out that info. Alternatively, we could just send the info in pre-commit hook. The chances of that transaction commit failing are close to 0.

​Thanks! That was a good dicussion. Cleaner solution coming soon!

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