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: adds bundle/content deploy and task management #183

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

tdstein
Copy link
Collaborator

@tdstein tdstein commented Apr 25, 2024

Resolves #110
Resolves #111
Resolves #112

@tdstein tdstein force-pushed the tdstein/110-bundle-deploy branch 2 times, most recently from a7161f1 to 8fb8e09 Compare April 25, 2024 20:01
Base automatically changed from tdstein/108-bundles-create to main April 25, 2024 20:02
@tdstein tdstein force-pushed the tdstein/110-bundle-deploy branch from 8fb8e09 to 18da02e Compare April 25, 2024 20:04
Copy link

github-actions bot commented Apr 26, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1018 982 96% 80% 🟢

New Files

File Coverage Status
src/posit/connect/tasks.py 100% 🟢
TOTAL 100% 🟢

Modified Files

File Coverage Status
src/posit/connect/bundles.py 100% 🟢
src/posit/connect/client.py 100% 🟢
src/posit/connect/content.py 100% 🟢
TOTAL 100% 🟢

updated for commit: 75380ac by action🐍

@tdstein tdstein force-pushed the tdstein/110-bundle-deploy branch from 027f722 to 780ca20 Compare April 26, 2024 14:55
@tdstein tdstein force-pushed the tdstein/110-bundle-deploy branch from 780ca20 to e705541 Compare April 26, 2024 14:55
@tdstein tdstein marked this pull request as ready for review April 26, 2024 14:55
src/posit/connect/bundles.py Outdated Show resolved Hide resolved
src/posit/connect/tasks.py Outdated Show resolved Hide resolved
None
"""
while not self.is_finished:
self.update(wait=sleep)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the API supports this, but it's odd to me that we'd push the sleep to the server. I.e. with wait_for(5), each request is going to take 5 seconds to respond. Seems odd to consume server resources for that waiting. Why not do the sleeping on the client side and always send wait=0 (which is the default anyway)?

The other thing you might do is pass the first argument, set to len(self.output) so that you're only returning newly generated lines of log output. You'd have to do something different than just super().update(**result) with that, have to be sure to append output and update the rest. But I could see that for a large task (IDK, lots of package installation output), not fetching all lines every time could be important.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same reaction when developing this. The one advantage of the current implementation is that the API will return once the task is finished. So, if you call with wait=60, the method will return in less than 60 seconds if the task finishes.

But I also agree. It seems like an anti-pattern to consume server threads waiting for the task to complete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added #184

@tdstein tdstein merged commit 4fa2102 into main Apr 29, 2024
7 checks passed
@tdstein tdstein deleted the tdstein/110-bundle-deploy branch April 29, 2024 15:12
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.

Create content (epic) Task polling Deploy a bundle
2 participants