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 space parameter for Posit Cloud #983

Merged
merged 10 commits into from
Sep 12, 2023
Merged

add space parameter for Posit Cloud #983

merged 10 commits into from
Sep 12, 2023

Conversation

mslynch
Copy link
Contributor

@mslynch mslynch commented Aug 28, 2023

Resolves #961.

cc @scubastevew

@mslynch mslynch requested review from hadley and aronatkins August 28, 2023 21:54
@mslynch mslynch marked this pull request as ready for review August 28, 2023 21:54
Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

The Connect client needs to be updated.

Error in client$deployApplication(application, bundle$id, space) : 
  unused argument (space)

The #961 issue also suggests creating a function to list the available spaces; is that arriving, as well?

R/client-shinyapps.R Outdated Show resolved Hide resolved
R/client-cloud.R Show resolved Hide resolved
R/client-cloud.R Outdated Show resolved Hide resolved
R/client-cloud.R Outdated
# in case the source cloud project is a temporary copy, there is no
# content id. The output will be published without a space id.
if (!is.null(currentProjectId)) {
json$project <- currentProjectId
Copy link
Contributor

Choose a reason for hiding this comment

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

This change does not add the json$project when provided an incoming spaceId. Is it needed in this case?

Compare this change to the change in deployApplication() -- there, the space associated with the current project is never used, while the current project id is used unconditionally (even when there is spaceId argument).

What is the expectation for the project and space fields provided to the different API calls? I'm not familiar with the APIs that are being called -- more details would be really helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change does not add the json$project when provided an incoming spaceId. Is it needed in this case?

Thanks, it is needed here – pushed a commit to change this logic. If we're executing inside a Posit Cloud project, we always want to link the output to that output through project. For the space, we want to update it if there's a specific space passed in through deployApp. If there's no space when we create the application, it should be set to the space of the project.

R/client-cloud.R Outdated
currentProjectId <- getCurrentProjectId(service, authInfo)
if (!is.null(currentProjectId)) {
outputPatchData$project <- currentProjectId
Copy link
Contributor

Choose a reason for hiding this comment

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

This is different logic than is used when creating; intentional? In this case, we could end up using currentProjectId from the environment and the spaceId argument. When creating, the spaceId argument is only used when there is no currentProjectId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the logic as-written makes sense (at least to me). When you deploy, it should definitely set the space based on the space of the environment project. But on redeploys, it would probably be more confusing for projects to move between spaces without providing the argument.

@m-- Any other thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also cc @kippandrew ^

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense to me.

tests/testthat/test-client-cloud.R Outdated Show resolved Hide resolved
@mslynch
Copy link
Contributor Author

mslynch commented Sep 12, 2023

The #961 issue also suggests creating a function to list the available spaces; is that arriving, as well?

Probably, likely in another PR.

@mslynch mslynch merged commit 21b1f53 into main Sep 12, 2023
@mslynch mslynch deleted the space-parameter branch September 12, 2023 16:58
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.

Add space parameter to deploy*() functions for specifying a destination when publishing to Posit Cloud
3 participants