- Notifications
You must be signed in to change notification settings - Fork85
refactor(connect): use v1 APIs for create/upload/deploy#1280
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
| currentUser=function() { | ||
| GET(service,authInfo,"/users/current") | ||
| # All callers only need $id and $username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This comment is just some research for#1278
| addToken=function(token) { | ||
| POST_JSON(service,authInfo,"/tokens",token) | ||
| POST_JSON(service,authInfo,unversioned_url("tokens"),token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
API not changed, just using theunversioned_url() wrapper, similar to what we do inconnectapi. Not only does it centralize and standardize the URL construction logic, which had a couple of different flavors in this file alone, it makes it easier to grep the codebase to see which calls are using v1 APIs and which aren't.
| guid=result$guid, | ||
| url=result$content_url, | ||
| # Include dashboard_url so we can open it or logs path after deploy | ||
| dashboard_url=result$dashboard_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
By includingdashboard_url here, we can construct the URLs that we were previously fetching from a "config" endpoint. Presence of "dashboard_url" in the response here marks it as being from Connect.
| uploadApplication=function(appId,bundlePath) { | ||
| path<-file.path("/applications",appId,"upload") | ||
| uploadBundle=function(contentGuid,bundlePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Function is renamed and swapped to taking guid. Other similar functions for shinyapps and Connect Cloud already refer to bundles.
| wait | ||
| ) | ||
| response<- GET(service,authInfo,path) | ||
| # ick, manual url construction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I started refactoring this because I thought we could just lethttr::GET() handle the query string assembly, but as it turns out,GET() here is not fromhttr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This might be a separate PR regardless, but IIUC one of the reasons we rolled our ownGET was to support configurations that are no longer a real issue (IIRC, though my searching just now didn't turn up anything elucidating, there was a period where we needed to support less-than-standardly-secure http connections). I did find#762 which started reducing the backends...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Interesting read. I made#1281 as a next step there.
| } | ||
| # These functions should move to client-shinyapps.R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just an observation, everything else is stashed in aclient object, except this one and the function below it. Not touching the shinyapps code in this PR though.
| propertyValue<-properties[[i]] | ||
| # dispatch to the appropriate client implementation | ||
| if (is.function(client$configureApplication)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a shinyapps function, but the onlyconfigureApplication() method was in the Connect client, and it didn't take these arguments (and didn't actually configure anything). Deleted.
| if (deploymentSucceeded) { | ||
| url<- paste(application$dashboard_url,"access",sep="/") | ||
| }else { | ||
| url<- paste(application$dashboard_url,"logs",sep="/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is literally what the oldconfig endpoint does, returns these two URLs.
Uh oh!
There was an error while loading.Please reload this page.
Closes#1277
Connect integration tests cover all lines modified, so we can have assurance that the APIs are being called and not erroring. Having made this change recently in
rsconnect-python, I believe this is safe, and that the v1 APIs (I was going to say "new APIs" but they've actually been there for years) work close enough to the previous ones, at least in the ways they're being used here.