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

DEVEX-2418 #1409

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

DEVEX-2418 #1409

wants to merge 3 commits into from

Conversation

vkuban-dx
Copy link
Collaborator

Fix run of globalworkflow when the underlying workflow will be searched in dx env Current workspace instead of its onw container in the given region.

Fix run of globalworkflow when the underlying workflow will be searched in dx env Current workspace instead of its onw container in the given region.
@vkuban-dx vkuban-dx added bug python Pull requests that update Python code labels Oct 18, 2024
dxid=underlying_workflow['workflow'],
project=underlying_workflow['resources'],
)
self._workflows_by_region[region] = dxworkflow
Copy link
Collaborator

Choose a reason for hiding this comment

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

We previously set self._workflow_desc_by_region variable to the DXWorkflow

With this change we set self._workflows_by_region[region]

Why are we no longer setting self._workflow_desc_by_region? It is used in describe_underlying_workflow.

Copy link
Collaborator Author

@vkuban-dx vkuban-dx Oct 21, 2024

Choose a reason for hiding this comment

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

There was a confusion between 2 attributes, see def __init__()

        # caches for underlying workflow instances and descriptions per region
        # (they are immutable for a given global workflow)
        self._workflows_by_region = {}
        self._workflow_desc_by_region = {}

This method should only cache the workflow instances, not descriptions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested that the caching in both cases still work as expected? If so, then LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I did.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are concerned about the behavior - you can wrap those in try ... except. Otherwise - could you identify the regression test that monitors this or maybe create one?

@vkuban-dx vkuban-dx requested a review from kpjensen October 21, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants