-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improved support for RMarkdown sites #2466
Conversation
…cluded, when there is a site configuration file present
…ategory as site for the resulting manifest json file that is included in the deployment bundle
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.
Code looks great, but I raised a few questions. I haven't tried to verify the functionality yet, that is my next step (and I'll creae a new comment with the result).
I was able to verify the functionality for the
I'm having problems getting the bookdown demo to run. Having problems installing packages within the directories of that repo, almost as if my GITHUB_PAT was invalid. I even went and generated a new one, set it up in my environment and made sure the R sessions were started from terminals with that change to my I'm also guessing a lot as to what would be the entrypoint for many of these content types... In the case of I'm unsure if my experience with bookdown is a user error or an issue with my configuration. I was able to render the bookdown site, with the changes you specified for |
@sagerb In that area I only run |
…iguration files. Compare the filename as is since site configuration is looked for only on the project root
2b259e7
to
7b8c109
Compare
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.
Code changes looked good, thanks!
Further questions on the exclusions, but we're close... :-)
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.
The RMD site worked beautifully for me. I did have to select a .Rmd
file for the inspection to work. Selecting _site.yml
created an unknown type configuration with no included files. That is acceptable, perhaps a follow-up would be to allow that file to work for inspection just to cover all our bases.
Once I figured out some weirdness with renv
on my system installing bookdown
I was able to deploy with no issues. Similar to the comment above about using _site.yml
as the entrypoint, using _bookdown.yml
gives type: 'unknown'
and could be a nice follow-up issue. Using a Rmd
works perfectly.
I'll need to update the CHANGELOG in #2472 as reading the description gave me a different impression on how this worked. I'll get that updated before I mark it as ready to review.
Including all but a few files for sites makes sense to me, but understandably we got some push back when we initially had files = ['*']
in the default configuration due to security concerns / accidentally deploying certain files.
The logic here includes a .env
file for example which could contain environment variables in a pretty common use case.
We should consider this carefully, and perhaps tighten up the file inclusion to only match certain file extensions.
The generated files
is the only bit of this PR I'm concerned with. The thorough testing you have done here makes it rock solid deployment-wise well done 👍
Looking forward to chat about this in our standup, meanwhile...
That's a good point! Specially when it is possible to select |
…up configuration as R Markdown Site
Pushed a new commit so that a configuration for R Markdown Site projects is created when picking |
…le for R Markdown Sites when config is created
9c81888
to
4e7a4f5
Compare
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.
Fantastic PR. Thank you very much @marcosnav for the changes, discussions, and the thorough testing. I tested with the two examples (rmd-site
and the Bookdown demo) and both worked beautifully after the few changes you made.
Intent
Update
The direction of this PR changed, it isn't automatically including the files and assets that could be required by the project. This change of direction after discussing with the team and surfacing previous feedback from user testing in which users might prefer to add things manually, instead of including files that could represent a security risk, (e.g: pushing secrets unintentionally)
Don't be alarmed:
From the 53 files shown in this PR 98% are artifacts used for testing, no need to code review those under
.../inspect/detectors/testdata/*
.This PR introduces some changes to improve the experience of deploying RMD sites.
Fixes #1643
Type of Change
Approach
Connect requires the
manifest.json
file to specifycontent_category
assite
when the document is going to be a website, the common cases for this are projects using_site.yml
configurations or_bookdown.yml
projects.The approach in this PR when there is a
_site.yml
or_bookdown.yml
file present:manifest.json
for the deployment bundle includes the category ofsite
.User Impact
Deployment of RMD sites should have a much better out of the box experience.
Automated Tests
Added and updated tests for the generated configuration and it's files, and for the update of the manifest file for the content category.
Directions for Reviewers
Update
As mentioned above, with the change of direction in this PR, assets and files will need to be added manually after creating the deployment.
Aside from possible local renv requirements, the following examples can be used to try this out, sites should be easily deployed without much intervention after creating the deployment with Publisher:
xelatex
system dependencies is not worth the work to test this, plain GitBook or HTML is enough. Updating the_output.yml
to the block of content shown below does the trick.Checklist