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

chore(orchestration): prevent throwing on already defined workflow #5337

Merged
merged 18 commits into from
Oct 18, 2023

Conversation

adrien2p
Copy link
Member

What
At the moment, when importing something from medusa entry point, if two medusa packages are installed because of a plugin, the evaluation of the import can end up throwing that a workflow is already defined. To prevent that from happening, we can just not throw if it is already defined and just return prematurely and make it idempotent.

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2023

🦋 Changeset detected

Latest commit: b4542f4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@adrien2p adrien2p marked this pull request as ready for review October 10, 2023 08:52
@adrien2p adrien2p requested a review from a team as a code owner October 10, 2023 08:52
@vercel
Copy link

vercel bot commented Oct 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Oct 18, 2023 9:33am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Oct 18, 2023 9:33am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Oct 18, 2023 9:33am

@adrien2p
Copy link
Member Author

@carlos-r-l-rodrigues could you have a look at this pr please

Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues left a comment

Choose a reason for hiding this comment

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

What if we call WorkflowManager.unregisterAll() right before exporting it?
I'm affraid that removing this validation, if new workflows are generated using the same name, it will be hard to debug what is causing it to not work as expected.

Or create another static property to flag if it's being imported twice and then we can just return.

@adrien2p
Copy link
Member Author

Maybe instead we should get all workflows to register and then register them all allowing us to identify duplicated. The problem here is that if you have multiple medusajs/medusa package installed because you are working locally or because a package has a deps to it, then the same workflows are re registered. Because rhe import comes from the entry point and the way it is at the moment some import end up evaluating end point which import the workflow and try to re register it

@olivermrbl
Copy link
Contributor

Is there a way we can detect if a workflow is an exact duplicate?

@adrien2p
Copy link
Member Author

I would say checking the flow step definition ?

@olivermrbl
Copy link
Contributor

If we can, would it make sense to allow for "re-registration" of identical workflows and otherwise throw?

@adrien2p
Copy link
Member Author

I think it is reasonable, ill do that

@carlos-r-l-rodrigues
Copy link
Contributor

we can only check the definition, not the handlers. But I believe this is enough

@adrien2p
Copy link
Member Author

@olivermrbl @carlos-r-l-rodrigues wdyt?

Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/orchestration/src/workflow/workflow-manager.ts Outdated Show resolved Hide resolved
Co-authored-by: Oli Juhl <59018053+olivermrbl@users.noreply.github.com>
@adrien2p
Copy link
Member Author

@olivermrbl can I put an auto merge flag?

@olivermrbl
Copy link
Contributor

Yep!

@kodiakhq kodiakhq bot merged commit e47461d into develop Oct 18, 2023
12 checks passed
@kodiakhq kodiakhq bot deleted the chore/workflow-manager branch October 18, 2023 09:47
@github-actions github-actions bot mentioned this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants