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

Enable detection and deployment of Quarto scripts #2424

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Conversation

marcosnav
Copy link
Collaborator

@marcosnav marcosnav commented Nov 11, 2024

Intent

This PR introduces a few changes required to allow Python and R files to be considered as entrypoints for Quarto rendered scripts.

Also, renaming the content type from quarto to quarto-static on many places to avoid future confusion, since Connect handles the type as quarto-static, within publisher code we should try to stick to quarto-static as well.

Fixes #1208

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Automated Tests

  • Updated and added unit tests as needed
  • Also, had to update e2e bats tests related to Quarto

Directions for Reviewers

This can be tested using the jumpstart examples "Quarto Script Python" and "Quarto Script R", choosing the .py and .R files should create the project configuration toml with type = "quarto-static".

Deployment should work too, TOML schema validations might show warnings until this PR is merged and the updated schema JSON file, that now includes quarto-static as a valid type, lands main.

Checklist

@marcosnav marcosnav force-pushed the mnv-quarto-scripts branch 2 times, most recently from 896232e to dc97e2a Compare November 12, 2024 12:53
@marcosnav marcosnav marked this pull request as ready for review November 12, 2024 13:48
@jonkeane
Copy link
Collaborator

Also, renames the content type quarto to quarto-static on many places to avoid confusion, since Connect handles the type as quarto-static, within publisher code we should try to stick to this type name as well.

Oh interesting! Is this true on Connect as well? If yes, is that documented anywhere? I went looking for this specifically and honestly couldn't even find that script -> quarto-static in our docs! I do very much prefer quarto to quarto-static from a readability standpoint, but want to make sure that that's in line with Connect and we're not doing something sliiiiightly different that'll bite us down the road.

Deployment should work too, but the TOML schema validation might get in the way until this PR is merged and the updated schema JSON file, that now includes quarto-static as a valid type, lands main.

I can RTFS / RTFPR, but is this validation still blocking a deployment or have we lifted that restriction and it's only an advisory "this isn't in the schema I know about"?

@marcosnav
Copy link
Collaborator Author

Oh interesting! Is this true on Connect as well? If yes, is that documented anywhere? I went looking for this specifically and honestly couldn't even find that script -> quarto-static in our docs! I do very much prefer quarto to quarto-static from a readability standpoint, but want to make sure that that's in line with Connect and we're not doing something sliiiiightly different that'll bite us down the road.

Yes, that is how Connect identifies this content type. We use quarto only on Connect UI for a broader content search, pulling both quarto-shiny and quarto-static.

Worth noting that for other types we have this kind of specificity, for example:

  • we don't have a just python type there is python-api python-dash python-streamlit python-bokeh python-fastapi
  • same for rmd types, we have rmd-shiny rmd-static
  • and jupyter types, we have jupyter-static jupyter-voila

There is a slight mention about quarto-static within search docs,
https://docs.posit.co/connect/user/viewing-content/#filtering-content-by-type-or-visibility

...but the full list of types can be found on API docs, specifically the endpoints related to the content schema
https://docs.posit.co/connect/api/#get-/v1/content/-guid-

It is not common for the user to determine the content type before hand, until now with Publisher. Previously, Connect would determine the type of content based on the bundle uploaded and deployed. Seems the publisher is the first tool making use of the content type identifier, in this case within the configuration toml, before pushing any bundle to Connect. I mention this because this seem to me to be the reason why the content type enumeration is not that prominent in the docs.

@marcosnav
Copy link
Collaborator Author

I can RTFS / RTFPR, but is this validation still blocking a deployment or have we lifted that restriction and it's only an advisory "this isn't in the schema I know about"?

I just double checked this, and you should be able to do deployments, the TOML schema linting will throw warnings until the PR lands, but that's all

@jonkeane
Copy link
Collaborator

...but the full list of types can be found on API docs, specifically the endpoints related to the content schema
https://docs.posit.co/connect/api/#get-/v1/content/-guid-

nods I did find this, and there is no mention of script in that block at all 😱 quarto-static - A [Quarto](https://quarto.org/) document or site. I guess this is technically true if one assumes that a script is a document, but I doubt most people would make that connection.

Also, renames the content type quarto to quarto-static on many places to avoid confusion, since Connect handles the type as quarto-static, within publisher code we should try to stick to this type name as well. [+ some of your responses as well]

OH OH OH. I read this backwards, sorry! I looked at the code and see that this is changing where the publisher code used to just use quarto everywhere we should have used quarto-static, we now use quarto-static This makes much more sense. Sorry for my confusion.

@marcosnav
Copy link
Collaborator Author

OH OH OH. I read this backwards, sorry! I looked at the code and see that this is changing where the publisher code used to just use quarto everywhere we should have used quarto-static, we now use quarto-static This makes much more sense. Sorry for my confusion.

Oh got it, thanks for the comment I updated the PR description to be more clear about this 👍

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Tested our Quarto content in the sample-content directory which includes a plain Quarto file plus R and Python runtime examples.

I also tested the examples in the rendered scripts docs. Everything deployed beautifully.

I also like the change to quarto-static to match Connect and all the testing you did here 👍

Fantastic work

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Wait I'm realizing that internal/schema/schemas/posit-publishing-schema-v3.json does not contain the change from quarto to quarto-static. Shouldn't that be included?

Marking as "request changes" just to be sure since I approved already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the type section under General settings also needs an update

@dotNomad
Copy link
Collaborator

dotNomad commented Nov 12, 2024

Wait I'm realizing that internal/schema/schemas/posit-publishing-schema-v3.json does not contain the change from quarto to quarto-static. Shouldn't that be included?

🤦 Nope I just can't read the files list in GitHub PRs apparently. Both of these files contain the quarto -> quarto-static change in type. It does look like there were some other changes needed in those files though. I have a PR open against this one with some of the things I caught including the docs change.

There are two open questions I have:

  • Should we update the schemas to v4 since these changes are not additive?
  • Should we support quarto as a type as well for users who are already using Publisher?

@dotNomad
Copy link
Collaborator

dotNomad commented Nov 12, 2024

After a quick discussion in stand-up:

  • We should still support the quarto type for those users already deploying Quarto content with Publisher
  • This would make the schema change additive so the version can stay the same
  • We should mark the quarto type deprecated and that it uses quarto-static under the hood in our configuration docs so it is more clear what is happening with that type

I'll start adding that to my PR #2426

@marcosnav
Copy link
Collaborator Author

marcosnav commented Nov 13, 2024

Thanks @dotNomad ! Merged #2426 into this PR, absolutely needed those changes.

About bumping schemas to v4, with your changes in, still supporting quarto content type, the changes feel additive and I think bumping the schema is not necessary. I have no strong opinion though, happy to bump it to v4 if you think it makes more sense.

This would make the schema change additive so the version can stay the same

👍

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Thanks for merging in my branch, and for the work on this PR I think it is looking great!

@marcosnav marcosnav merged commit 392e648 into main Nov 13, 2024
14 checks passed
@marcosnav marcosnav deleted the mnv-quarto-scripts branch November 13, 2024 21:00
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.

Support quarto-rendered scripts
3 participants