Skip to content

Expose timeout parameter to packs.install on st2 client pack install #5174

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

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

Conversation

hnanchahal
Copy link
Contributor

@hnanchahal hnanchahal commented Mar 2, 2021

Added timeout parameter for pack install st2 client run to help with long running installs. This was already expose in packs.install action but st2client was not updated to reflect the change.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Mar 2, 2021
@Kami
Copy link
Member

Kami commented Mar 2, 2021

I believe you also need to update st2common/st2common/openapi.yaml.j2, specifically PacksInstall object and also the corresponding API handler code in st2api/st2api/controllers/v1/packs.py.

@Kami
Copy link
Member

Kami commented Mar 2, 2021

Thanks for the contribution.

Overall, I'm fine with this change, but as mentioned above, it needs some more work + ideally at least the unit test for the API layer (that one shouldn't be too hard to add).

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Mar 2, 2021
@cognifloyd
Copy link
Member

We just reformatted the code with black. (Hooray!) And this PR got caught in the cross fire too. (Arrgh!)
Please merge master into your branch, resolve the conflicts (Ouch! Sorry!), and reformat with black (I recommend running pre-commit install after you've merged master so that black reformatting happens automatically on commit).

@stale
Copy link

stale bot commented Jun 9, 2021

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.

@stale stale bot added the stale label Jun 9, 2021
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ cognifloyd
❌ hnanchahal
You have signed the CLA already but the status is still pending? Let us recheck it.

@cognifloyd
Copy link
Member

@hnanchahal I just merged in master and dealt with reformatting.
Would you please sign the CLA so we can move this PR forward?

@cognifloyd cognifloyd added the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Apr 1, 2022
@cognifloyd
Copy link
Member

Rephrasing what Kami said, to finish up this PR, you need to add the server-side / API changes.
Though the packs.install action has a timeout parameter, the API layer can't pass it on until it knows about it.

So, you've added the CLI changes and the spec changes, the last pieces are:

  • change st2api/st2api/controllers/v1/packs.py (PackInstallController)
  • add API tests in st2api/tests/unit/controllers/v1/test_packs.py

And sign the CLA :) If you can get this done within a week or two I'll merge it in time to release 3.7.0. Cheers!

@andrewhirasawa
Copy link

any update here? We have a pack with a long installation process that takes longer than the default 600s. It would be nice to use this feature to set the timeout to a custom length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API CLI component:st2client service: api size/M PR that changes 30-99 lines. Good size to review. status:needs changes status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. status:needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants