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

feat: request execution mode #3200

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

lohxt1
Copy link
Collaborator

@lohxt1 lohxt1 commented Sep 26, 2024

fixes #3104

extension of pr #3105

moving the getExecutionMode fn to req object instead of bru object

req.getExecutionMode()

values: standalone | runner | cli

@helloanoop
Copy link
Contributor

@nikischin Can you review this PR?

Copy link

@nikischin nikischin left a comment

Choose a reason for hiding this comment

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

Excellent, thank you so much for this modified implementation!

I reviewed code and tested in UI in both developer and safe mode and it does work as intended. I haven't tested in cli, so maybe someone can confirm for cli?

Please include Fixes #3104 in the description of the pr so the ticket will be automatically closed :)

@helloanoop helloanoop merged commit 25f43f1 into usebruno:main Sep 26, 2024
2 checks passed
@helloanoop
Copy link
Contributor

Merged!

Thank you @lohxt1 and @nikischin !

@helloanoop helloanoop mentioned this pull request Sep 26, 2024
1 task
@helloanoop
Copy link
Contributor

@nikischin I am thinking that for now, we should remove the cli from the options returned.
Even when the run is being done in CLI, it should still return standalone | runner based on whether a single request is being run or the a folder / collection is being run

An api to perhaps consider in the future is bru.getPlatform() that can return app|cli based on where the run is happening

Do you concur?

@nikischin
Copy link

nikischin commented Sep 26, 2024

@nikischin I am thinking that for now, we should remove the cli from the options returned.

Even when the run is being done in CLI, it should still return standalone | runner based on whether a single request is being run or the a folder / collection is being run

An api to perhaps consider in the future is bru.getPlatform() that can return app|cli based on where the run is happening

Do you concur?

Honestly, I have never used the cli so far. I might be interested in doing so in the future but did not have a need for it yet (if I would include it in some workflows, this would be the way to go).

So honestly I cannot really provide any valuable feedback on this one.

  • If there's different ways of how to execute the cli I would prefer the version of having standalone and runner in both cli and ui with a separate getPlattform().
  • If however in cli it does not make a difference (let's say it's always a batch, but some times a batch of only one - this was what I assumed in my initial implementation) I would recommend to leave the implementation as is.

Does this make sense? In the end you can decide better what's the best way to do, however if we change I would do so before releasing a new version as this would be a breaking change. I hope my feedback helped.

Edit: I checked the cli docs and the version you suggested with

An api to perhaps consider in the future is bru.getPlatform() that can return app|cli based on where the run is happening

would definitely make sense.

@nikischin
Copy link

@helloanoop @lohxt1 let me know if I should modify the implementation in order to reflext the changes suggested or if you will do it:

@nikischin I am thinking that for now, we should remove the cli from the options returned. Even when the run is being done in CLI, it should still return standalone | runner based on whether a single request is being run or the a folder / collection is being run

An api to perhaps consider in the future is bru.getPlatform() that can return app|cli based on where the run is happening

One detail which for me is unclear, if the getPlatform() would be attached to bru or the req object. From a user perspective it doesn't make a big difference, but for the implementation it could matter.

@lohxt1
Copy link
Collaborator Author

lohxt1 commented Sep 26, 2024

i'm leaning more towards req.getPlatform()

i will work on making the changes @helloanoop @nikischin

@helloanoop
Copy link
Contributor

Thanks for your feedback @nikischin !
We'll go with req.getExecutionPlatform() that returns app|cli based on where the request is being run

@lohxt1 will be make the updates for this.

@nikischin
Copy link

nikischin commented Nov 4, 2024

Thanks for your feedback @nikischin !

We'll go with req.getExecutionPlatform() that returns app|cli based on where the request is being run

@lohxt1 will be make the updates for this.

Any update on this @lohxt1 ? I would recommend implementing it before too many people start to use the current implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detection for runner in script
3 participants