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

Add support for shiny express apps #521

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Add support for shiny express apps #521

merged 6 commits into from
Nov 29, 2023

Conversation

wch
Copy link
Collaborator

@wch wch commented Nov 13, 2023

Intent

This PR adds support for Shiny express apps.

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

The main change was pretty straightforward. It involves copying the contents of one file from the shiny package, and then adding three lines of code that change the entrypoint and set an environment variable.

However, there were more changes that I made, with some help from @mmarchetti. The use of kwargs = locals(), and passing the kwargs along to RSConnectExecutor(**kwargs) was confusing and caused some problems, so I made the arguments explicit.

Automated Tests

I didn't add any tests.

Directions for Reviewers

This will require a close look to make sure that the extra_args being passed to RSConnectExecutor() contain the correct items. With the previous code, it was extremely difficult to tell which arguments from the kwargs were meant to go to RSConnectExecutor(), and which ones just happened to be along for the ride.

Checklist

  • I have updated CHANGELOG.md to cover notable changes.
  • I have updated all related GitHub issues to reflect their current state.

@wch wch requested a review from mmarchetti November 13, 2023 19:28
@wch
Copy link
Collaborator Author

wch commented Nov 13, 2023

That there is a type-checking error in CI -- this issue actually existed in the code before but wasn't picked up on by the type checker because it was obscured by passing in the kwargs; now that the arguments are explicit, the type checker is seeing the problem.

The problem is that in deploy_app(), cacert is typed as IO[Any], but then it is passed to RSConnectExecutor(), and the class constructor there expects cacert to be a str.

Looking at the code, I suspect that it should be str in both places, but I don't know for sure.

@wch
Copy link
Collaborator Author

wch commented Nov 14, 2023

I also didn't update the changelog because there wasn't a section since the previous release. Should I add one?

Copy link

github-actions bot commented Nov 16, 2023

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
4411 2857 65% 0% 🟢

New Files

File Coverage Status
rsconnect/shiny_express.py 26% 🟢
TOTAL 26% 🟢

Modified Files

File Coverage Status
rsconnect/main.py 56% 🟢
TOTAL 56% 🟢

updated for commit: d9e230a by action🐍

@karangattu
Copy link

Verified that deployments for shiny express app works as expected for connect, shinyapps.io and posit.cloud.
Connect url for deployed app - link
Shinyapps.io url for deployed app - link
Posit.cloud url for deployed app - link

@karangattu
Copy link

Tests for deploying shiny express apps via the rsconnect-python package to connect will be added in the next week.

@karangattu
Copy link

The tests for this will live in py-shiny and will be merged once rsconnect main has the PR code. All manual tests are done and the feature works as expected. PR can be merged.

@wch wch merged commit cf170a3 into master Nov 29, 2023
14 checks passed
@wch wch deleted the shiny-express branch November 29, 2023 21:42
@mmarchetti mmarchetti mentioned this pull request Nov 30, 2023
3 tasks
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.

3 participants