Improve Quarto websites projects inspection and generated configuration #2455
Conversation
… to the configuration and allowing to use quarto yml files as entrypoints
marcosnav
requested review from
dotNomad,
kgartland-rstudio and
sagerb
as code owners
| @@ -22,11 +22,11 @@ type QuartoDetector struct { | |||
| log logging.Logger | |||
| } | |||
|
|
|||
| func NewQuartoDetector() *QuartoDetector { | |||
| func NewQuartoDetector(log logging.Logger) *QuartoDetector { | |||
There was a problem hiding this comment.
Noticed the previous log implementation in this struct wasn't logging because it was not the initialized logger instance with a pre-defined output. Updated the signature to receive the logger instead of creating a new one.
| @@ -36,25 +36,27 @@ type quartoMetadata struct { | |||
| Server any `json:"server"` | |||
| } | |||
|
|
|||
| type quartoProjectConfig struct { | |||
There was a problem hiding this comment.
Pulled out this config object for re-usability purposes.
|
@marcosnav The code within the PR is looking good, but I started hitting some issues with the quarto examples within I started with In this case, I saw The initial error I saw within the render logs on Connect was that If we are going to include all of the referenced files automatically for quarto projects, (at least the ones that quarto inspect does tell us about), then I think you'll want to make sure you parse out all of the locations in which the inspect results will return file references. Do you think it would be a good idea to source the schema from the quarto code itself, to determine what is returned by inspect (with all of the file references it could have)? Let me know if you agree with me that we should address these additional file references within your PR. Thanks so much! |
|
@sagerb Thanks for trying it out and noticing that. I tested out mainly the jumpstart examples and a couple other projects, I'll take a look to this other bundles to see other cases that we might be missing. I do think that it makes sense to address some of these other file inclusions on this PR, I'll take another pass to the schema we have and see what we are missing. |
|
Also, Quarto 1.6 was just released and it now supports a new |
|
@marcosnav Do you think we need to first capture the version of quarto and then deserialize into a version specific structure? Or are you seeing that they are simply adding additional fields vs. removing/changing ones? |
@sagerb So far I haven't seen the need for versioning the structure, changes are additive. Specifically for the new |
|
@sagerb PR ready for another look, with some more adjustments to identify and add project related resources. |
|
Validation details from projects in repo:
|
|
Summary of validation findings: Problem #1: Not detecting Problem #2: We are seeing different inspection results depending if the entrypoint selected is Questions
|
|
@marcosnav See my findings above. I think you should address the two |
|
Thanks for the thorough testing @sagerb!
Good call on this one, I pushed a new commit addressing the config resources found when picking an individual .qmd file as entrypoint.
I don't think so, About Problem 2 -
|
This is exactly the place where we are stuck currently given the ecosystem of manifest.json + needing more information than the renv.lock file has to send up to connect. We would like to live in a Connect world where this isn't a requirement, but today it is. So today we do need to effectively be in a place (well the renv library does) where one could execute the code. But this isn't transitive: just because today we do effectively need to be in that state, that doesn't mean that executing the code is a requirement we should always assert/strive for in other situations or in the future if the manifest.json/renv.lock issue is solved.
I couldn't quite tell from the formatting if this was a quote of something somewhere or what. But, yes this sounds like a good idea. We almost certainly should not do it in this PR, but using |
There was a problem hiding this comment.
Thanks for making the latest set of changes. I've validated them and reviewed that code as well. Looks great!
I'm ok with putting off a discussion regarding the switch to simply always performing inspection upon the subdirectory of what has been selected as an entrypoint. It feels like something that would provide the same type of functionality as the IDE, so I'd recommend we investigate and discuss this further.
Intent
The goal of this PR is to improve the experience of Quarto website projects. Focused in two main points:
pre-andpost-scripts are now automatically picked up to be added to the configuration on project inspection._quarto.ymlas entrypoint and configuration will be properly generated.Fixes #2266
Type of Change
Approach
In order to allow points mentioned above, it was necessary to reorganize a bit the code that triggers the
quarto inspectcommand and that, sub sequentially, uses the output to generate the deployment configuration, having specific checks for_quarto.ymlpicked up as entrypoint and extending the inspect outputstructto account for directory scanning scenarios.User Impact
Users now will:
pre-andpost-scripts included - if any - to the configuration on Quarto project inspection._quarto.ymlas entrypoint and configuration will be properly generated.Automated Tests
New tests were created to cover this new functionality.
Directions for Reviewers
Try publishing a Quarto website project, even better if it has
pre-renderandpost-renderscripts to confirm such files are included in the resulting configuration.The jumpstart example of a Quarto website can be used as example.
Checklist