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

setNextRequest alternative implementation #8132

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

jackkav
Copy link
Contributor

@jackkav jackkav commented Oct 26, 2024

@ihexxa as I described in our meeting last week here is an alterative way to implement a workflow queue

Aims:

  • Seperate the queue contruction from the implementation so we can share code.
  • use a guard pattern at the top of the loop to limit if else clauses
  • use early returns in bail cases
  • group bail skip and setNextRequest together in order to aid readability, and potentially unit test

Could still be improved but I wanted to show you what I had in my head to help us improve the runner.tsx implementation and achieve a little more code reuse.

Let me know what you think

TODO:

  • figure out why nextRequestIdOrName comes back empty
  • add skip request
  • add support for setNextRequest(null)
  • refactor runner.tsx to share some code with this approach

Closes INS-4604

@jackkav jackkav force-pushed the set-next-request-workflow-implementation-cli branch from 771563a to 992d016 Compare October 26, 2024 22:48
@ihexxa
Copy link
Contributor

ihexxa commented Oct 30, 2024

@jackkav I’ve been tinkering with this PR locally these days, the workflowQueue solution looks cool. Although I find something worth discussing:

One is related to the setNextRequest, as the workflowQueue is modified in iteration, it requires us to be pretty careful when removing skipped requests, adding the current request. For example, if we try to find the next target request in a 1-dimension array, the next request could point to one request of the next iteration, which is incorrect. So I'm concerning about this pattern could be even harder to control.

Besides, the workflow solution should also work with the skipRequest interface and the bail option when they are enabled at the same time. I believe that it would also quickly become complicated when all these requirements are implemented.

Another thing is the behavior and code sharing, as the CLI’s behavior should be consistent with UI or the similar product, theoretically, most of the workflow logic should be shared, so sharing the setNextRequest is a very small part, which seems not enough, sometimes I noticed differences.

I will have more checks on the CLI implementation and see the best way to fix the above.

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.

2 participants