Disallow deploying `.qmd` files with `rmd-*` app modes #595

toph-allen · 2022-06-14T19:40:27Z

Intent

Previously, Quarto documents and projects would deploy as rmd-static or rmd-shiny if they were deployed without Quarto info available (either via running quarto inspect or via the metadata argument).

With this PR, rsconnect will stop with an error if that condition occurs, and advise the user to pass the path to a Quarto binary to the quarto argument.

Fixes #594

Approach

Previously, we would infer one set of rmdFiles that would contain both Quarto and R Markdown files, and decide between rmd-* and quarto-* app modes based on the presence or absence of quartoInfo, which is the inferred Quarto info from either running Quarto Inspect or unpacking metadata.

Now, we find .Rmd and .qmd files separately. The list of file names is stored in rmdFiles and qmdFiles separately, and a logical vector of whether they're runtime: shiny is stored in shinyRmdFiles and shinyQmdFiles. We also look for ^_quarto.y(a)?ml$.

A deployment requires Quarto if… I was going to write out the condition, but the code is more readable than English.

  hasQuartoSupportedFiles <- any(length(qmdFiles) > 0, length(rmdFiles > 0))
  requiresQuarto <- (hasQuartoSupportedFiles && hasQuartoYaml) || length(qmdFiles) > 0

Any .qmd files make us require Quarto info. or, if we have Quarto-compatible files and a _quarto.yml, we need Quarto info. In those cases, if we don't have it, we raise an error.

After this point, we take the presence of the quartoInfo variable to determine whether we use quarto or rmd app modes, in each locations where we assign *-shiny and *-static. This still will let you deploy R Markdown files as Quarto content, but prevents you from deploying Quarto content as non-Quarto.

The RStudio IDE was adding the line PackageCleanBeforeInstall: Yes to .Rproj files for a while. Now it apparently removes that line. I committed the change, because it happened with every install of a package.

Automated Tests

Three tests were added:

Attempt to create a manifest for quarto-website-r, leaving the quarto argument NULL, and expect the error message.
Attempt to create a manifest for quarto-doc-none, leaving the quarto argument NULL, and expect the error message.
Attempt to create a manifest for test-reds/simple.Rmd, passing in quarto. Ensure that the manifest has the quarto-static app mode and knitr engine.

QA Notes

Install this branch of rsconnect from the RStudio IDE.

Quarto content in the tests/testthat directory should deploy correctly:

rsconnect::deployApp(appDir = "tests/testthat/quarto-website-r", server = "localhost", quarto = quarto::quarto_path())

Without passing the quarto argument, you should see an error:

rsconnect::deployApp(appDir = "tests/testthat/quarto-website-r", server = "localhost")

After testing, you'll want to go into the tests/testthat/quarto-website-r folder and remove the rsconnect folder that was created.

Deploying an R Markdown document from the IDE with this version installed should still deploy with an R Markdown app mode. (I tested this myself, but it's good to verify in a different environment.)

Checklist

If this PR adds a new feature, or fixes a bug in a previously released version, it includes an entry in NEWS.md

aronatkins

I'm unconvinced about the error when we have both Rmd and Qmd files.

aronatkins · 2022-06-14T20:36:31Z

R/bundle.R

+  shinyQmdFiles <- sapply(file.path(appDir, qmdFiles), isShinyRmd)
+
+  # Trying to deploy Quarto and R Markdown files simultaneously is an error.
+  if (length(rmdFiles) > 0 && length(qmdFiles) > 0) {


We should think about this error a bit more. It is possible to have a Quarto project that uses a document.Rmd file. Similarly, it is possible to have a Quarto project with a notebook.ipynb file.

It might be helpful if we captured an enumeration of file name collections and explained which combinations lead to different app modes, especially in the presence / non-presence of quartoInfo.

Here's one example:

_quarto.yml

analysis.Rmd

playground.ipynb

What content type should be used when this code is deployed to Connect? Why?

Similarly, what about:

report.Rmd with non-nil quartoInfo

Is this information more a signal to use Quarto or to use R Markdown?

Yeah, you raise a good point.

The app mode is inferred after we have put Quarto info into the format we desire, so by this point it's not clear whether it comes from running quarto inspect or as metadata passed in from the Quarto package.

I'm not sure what Connect would do with the first example you provide.

We had been using quartoInfo's presence as an indicator of user intent to deploy Quarto. If continue to rely on that signal, we could do something like the following.

with quartoInfo:

App mode is quarto-static, or quarto-shiny if any shinyRmdFiles or shinyQmdFiles.

without quartoInfo:

App mode is rmd-static or rmd-shiny, as would have been otherwise detected

Any qmdFiles or shinyQmdFiles results in an error?

(Thinking about this more. When Quarto renders R Markdown files, their chunk arguments are parsed as R Markdown would, I'm guessing.)

I agree, we should discuss tomorrow. I was about to make some changes, but I want to confirm we're on the same page before I do another round on this.

toph-allen · 2022-06-15T18:07:35Z

From a discussion today, the behavior of this code is going to change to the following:

Content will require Quarto metadata (quartoInfo) if we find any .qmds or a _quarto.yml in our deployment files.
If content requires quartoInfo, but no quartoInfo is available, deployment will stop with an error.
If we have quartoInfo, we will use quarto-shiny and quarto-static app modes instead of rmd-shiny and rmd-static app modes.

This will allow users to deploy non-.qmd files with Quarto, but will prevent them from deploying .qmd files without Quarto.

toph-allen · 2022-06-16T16:07:45Z

@aronatkins Discovered another bug in this while working on slides.

If you try to publish the "finished document" via the IDE, _quarto.yaml might be included by default, and if it is, we will detect that file and the error will occur.

[Edit] I'm not sure if this is an IDE bug (e.g. "Don't include _quarto.yml when deploying static content") or something we need to work around. Working around it might require larger restructuring of the inferAppMode() function.

toph-allen · 2022-06-28T17:09:52Z

The Quarto app mode is not correctly assigned when an appPrimaryDoc isn't present, it seems.

I'm duplicating this comment here so that discussion related to the PR itself can occur on the PR.

When I deployed the Rmd file, it deployed with a Quarto app mode.

> rsconnect::deployApp(".", appPrimaryDoc = "rmdfile.Rmd", quarto = quarto::quarto_path(), server = "rsc.radixu.com")
Preparing to deploy document...DONE
Uploading bundle for document: 5672...DONE
Deploying bundle: 13125 for document: 5672 ...
[Connect] Building Quarto document...

https://rsc.radixu.com/connect/#/apps/6cc07579-932b-430e-9399-fa3a3b67905b/info/5454

When I deployed without passing in appPrimaryDoc, it deployed with an R Markdown app mode.

toph-allen · 2022-06-28T17:33:52Z

The issue that @ChaitaC identified is due to a circular dependency in the way that rsconnect conducts its inference. The following code occurs in deployApp and writeManifest. The result of this is that the Quarto app mode will not be inferred if an appPrimaryDoc is not explicitly passed in.

In other words, deploying the same content with rsconnect::deployApp(".", appPrimaryDoc = "rmdfile.Rmd", quarto = quarto::quarto_path(), server = $CONNECT_SERVER) and rsconnect::deployApp(".", quarto = quarto::quarto_path(), server = $CONNECT_SERVER) will result in quarto-static and rmd-static app modes respectively.

This might not actually be a bug — this might be the correct functionality. I'm going to explore the topic in this comment.

The issue occurs due to the following structure, which occurs in deployApp() and writeManifest().

  quartoInfo <- inferQuartoInfo(
    appDir = appDir,
    appPrimaryDoc = appPrimaryDoc,
    quarto = quarto,
    metadata = metadata
  )

  logger("Inferring App mode and parameters")
  appMode <- inferAppMode(
      appDir = appDir,
      appPrimaryDoc = appPrimaryDoc,
      files = appFiles,
      quartoInfo = quartoInfo)
  appPrimaryDoc <- inferAppPrimaryDoc(
      appPrimaryDoc = appPrimaryDoc,
      appFiles = appFiles,
      appMode = appMode)

I'll describe the issue:

inferQuartoInfo() runs quarto inspect, first on the appDir to inspect projects, and then on the appPrimaryDoc to inspect single documents. It needs the appPrimaryDoc argument to know which file to inspect in a single-file deployment.
inferAppMode() uses the presence of quartoInfo to infer the app mode. If quartoInfo is `NULL, it does not consider Quarto app modes.
inferAppPrimaryDoc() needs the appMode to infer the primary document if none has been explicitly provided. Because this needs to go after inferAppMode(), we can't use an inferred primary document to infer a Quarto app mode.

We want to allow users to deploy R Markdown documents as Quarto content. However, calling deployApp(appDir, quarto = quarto) without specifying a primary document asks rsconnect to deploy a directory, not a single file. If the directory does not contain a _quarto.yml file is not a valid Quarto project. With this framing, deploying this with rmd-static is the correct course of action.

In other words, in this way of thinking appPrimaryDoc isn't just an optional argument to hint to rsconnect the correct primary doc. It's a part of the content specification, an appdDir = "dir" is different from `appDir = "dir", appPrimaryDoc = "doc.Rmd".

I think this is already the case, as even before Quarto's introduction inferAppMode used the explicit appPrimaryDoc to infer the app mode before inferAppPrimaryDoc used the appMode to guess the primary doc if none was passed in.

This doesn't mean that this is actually easy for users to understand, though. I'm not sure what the best way to proceed is. @aronatkins, do you have any thoughts?

jthomasmock · 2022-06-28T18:45:39Z

When I deployed the Rmd file, it deployed with a Quarto app mode.

 rsconnect::deployApp(".", appPrimaryDoc = "rmdfile.Rmd",
   quarto = quarto::quarto_path(), server = "rsc.radixu.com")

Preparing to deploy document...DONE
Uploading bundle for document: 5672...DONE
Deploying bundle: 13125 for document: 5672 ...
[Connect] Building Quarto document...

In my mind, specifying should always publish Quarto

format: html in the YAML even if a .Rmd
quarto = quarto::quarto_path() in rsconnect

It'd be very surprising as a user to specify a Quarto path and end up with a RMarkdown type document.

toph-allen · 2022-06-28T19:00:53Z

When I deployed the Rmd file, it deployed with a Quarto app mode.
 rsconnect::deployApp(".", appPrimaryDoc = "rmdfile.Rmd",
   quarto = quarto::quarto_path(), server = "rsc.radixu.com")

Preparing to deploy document...DONE
Uploading bundle for document: 5672...DONE
Deploying bundle: 13125 for document: 5672 ...
[Connect] Building Quarto document...
In my mind, specifying should always publish Quarto

format: html in the YAML even if a .Rmd

quarto = quarto::quarto_path() in rsconnect

It'd be very surprising as a user to specify a Quarto path and end up with a RMarkdown type document.

If I understand correctly, in this case, passing in the quarto argument would fail unless quarto inspect runs successfully, rather than falling back to other app modes? In this case, we would need new error messages for those failures.

I don't understand the first bullet point. Are you suggesting that we should force Quarto for R Markdown docs that output HTML, even if no Quarto arg is given? As the code is written right now, we can't do that without an explicit Quarto path, and we've tried to be careful about not changing the package's behavior in response to consistent inputs.

[edit] mentioning @jthomasmock

jthomasmock · 2022-06-28T19:11:03Z

If I understand correctly, in this case, passing in the quarto argument would fail unless quarto inspect runs successfully, rather than falling back to other app modes? In this case, we would need new error messages for those failures.

Yes, I think we'd rather throw an error on failure to treat as Quarto rather than silently publish as RMarkdown when the user specified Quarto.

I don't understand the first bullet point. Are you suggesting that we should force Quarto for R Markdown docs that output HTML, even if no Quarto arg is given? As the code is written right now, we can't do that without an explicit Quarto path, and we've tried to be careful about not changing the package's behavior in response to consistent inputs.

Sorry for lack of context!

In the YAML header:

format: html indicates Quarto
output: html_document indicates RMarkdown

aronatkins · 2022-06-29T13:59:17Z

One more piece of information: We have rsconnect::deployDoc to assist with document deploys. That path makes sure that the deployApp argument appPrimaryDoc receives a value.

aronatkins · 2022-06-29T14:05:30Z

We could expand inferQuartoInfo to take the primary doc and the files list.

inspect the directory and use its result when successful.
inspect the primary doc when non-NULL.
inspect the first doc in the files list when there is no explicit primary doc.

toph-allen · 2022-06-29T15:24:22Z

Per our discussion in today's sync, I'm going to move the current discussion to a new GitHub issue, and this PR has solved the problem it set out to solve.

# Conflicts: # NEWS.md

toph-allen added 5 commits June 14, 2022 15:35

Update quarto file handling, add tests

e3bced7

Fix typo

1f19eb8

Adjust language

ad8153a

The IDE now apparently removes this line from project files

62e3b4a

Update NEWS.md

6b1e2aa

toph-allen marked this pull request as ready for review June 14, 2022 19:57

toph-allen requested a review from aronatkins June 14, 2022 19:57

aronatkins reviewed Jun 14, 2022

View reviewed changes

toph-allen marked this pull request as draft June 15, 2022 17:49

Update conditions, adjust tests

7080f6f

toph-allen force-pushed the toph-disallow-qmd-as-rmd branch from 8036c59 to 7080f6f Compare June 15, 2022 19:35

Remove no-longer-needed test files

02024dc

toph-allen requested a review from aronatkins June 15, 2022 20:01

Update Quarto conditions

478476c

toph-allen marked this pull request as ready for review June 16, 2022 21:29

aronatkins approved these changes Jun 17, 2022

View reviewed changes

toph-allen closed this Jun 28, 2022

toph-allen reopened this Jun 28, 2022

aronatkins closed this Jun 29, 2022

aronatkins reopened this Jun 29, 2022

toph-allen mentioned this pull request Jun 29, 2022

rsconnect: infer Quarto info for document deployments where appPrimaryDoc is not specified #601

Closed

Merge branch 'main' into toph-disallow-qmd-as-rmd

1626933

# Conflicts: # NEWS.md

toph-allen merged commit f22e0a6 into main Jun 29, 2022

toph-allen deleted the toph-disallow-qmd-as-rmd branch June 29, 2022 20:27

This was referenced Feb 17, 2023

writeManifest() assumes a Quarto deployment in the presence of a .qmd #658

Closed

Mild refactoring of app metadata #673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow deploying `.qmd` files with `rmd-*` app modes #595

Disallow deploying `.qmd` files with `rmd-*` app modes #595

toph-allen commented Jun 14, 2022 •

edited by ChaitaC

Loading

aronatkins left a comment

aronatkins Jun 14, 2022

toph-allen Jun 14, 2022

toph-allen Jun 14, 2022

toph-allen commented Jun 15, 2022 •

edited

Loading

toph-allen commented Jun 16, 2022 •

edited

Loading

toph-allen commented Jun 28, 2022

toph-allen commented Jun 28, 2022 •

edited

Loading

jthomasmock commented Jun 28, 2022

toph-allen commented Jun 28, 2022 •

edited

Loading

jthomasmock commented Jun 28, 2022 •

edited

Loading

aronatkins commented Jun 29, 2022

aronatkins commented Jun 29, 2022

toph-allen commented Jun 29, 2022

Disallow deploying .qmd files with rmd-* app modes #595

Disallow deploying .qmd files with rmd-* app modes #595

Conversation

toph-allen commented Jun 14, 2022 • edited by ChaitaC Loading

Intent

Approach

Automated Tests

QA Notes

Checklist

aronatkins left a comment

Choose a reason for hiding this comment

aronatkins Jun 14, 2022

Choose a reason for hiding this comment

toph-allen Jun 14, 2022

Choose a reason for hiding this comment

toph-allen Jun 14, 2022

Choose a reason for hiding this comment

toph-allen commented Jun 15, 2022 • edited Loading

toph-allen commented Jun 16, 2022 • edited Loading

toph-allen commented Jun 28, 2022

toph-allen commented Jun 28, 2022 • edited Loading

jthomasmock commented Jun 28, 2022

toph-allen commented Jun 28, 2022 • edited Loading

jthomasmock commented Jun 28, 2022 • edited Loading

aronatkins commented Jun 29, 2022

aronatkins commented Jun 29, 2022

toph-allen commented Jun 29, 2022

Disallow deploying `.qmd` files with `rmd-*` app modes #595

Disallow deploying `.qmd` files with `rmd-*` app modes #595

toph-allen commented Jun 14, 2022 •

edited by ChaitaC

Loading

toph-allen commented Jun 15, 2022 •

edited

Loading

toph-allen commented Jun 16, 2022 •

edited

Loading

toph-allen commented Jun 28, 2022 •

edited

Loading

toph-allen commented Jun 28, 2022 •

edited

Loading

jthomasmock commented Jun 28, 2022 •

edited

Loading