fixes #1757 - renders if package is read-only

[dpastoor]

I've tested this in a pipeline on one of the self-hosted runners where gsm is installed globally as read only into /opt/rpkgs/gsm/gsm. Prior behavior was it would fail with a permissions error, now its passing.

[jonthegeek]

I'm at lunch but don't merge this. There's a better fix that we discussed while fixing the reports for the site.

[jonthegeek]

@jwildfire @lauramaxwell I'd be surprised if I passed a relative path and the write happened relative to my temp dir, which is what this fix would do. But the default rmarkdown::render() behavior (save relative to the source file) is also perplexing here. I think we should render either relative to the working directory or potentially relative to the working project (if there is one). In any case, if the user passes an absolute path via strOutpath (rather than relative), we should use that path (which is how things would behave even in the case of this version of the fix).

Does relative-to-working-directory seem like the right answer, or should we do something else? I can implement it, I just wouldn't do so quite how @dpastoor did.

@dpastoor As a work-around, if you pass an absolute path via strOutPath, Report_KRI() will respect that. You can use that to avoid this issue in workflows, etc.

[dpastoor]

@jonthegeek yes lets talk shortly in the zoom call about this. The biggest "issue" i have with this is in the CI/CD pipeline, this function is buried in the orchestration yaml a bit, making it harder to control the strOutput, especially for things like generating a temp name.

[lauramaxwell]

I'm working on an alternate solution that addresses both @jonthegeek and @dpastoor use cases. It will be ready to review/comment on by tomorrow morning.

[lauramaxwell]

I put up a proposed solution that should work for a temp context by checking permissions of the output directory and creating the temp structure if it is not writable. This also breaks apart specifying a custom file name (strOutputFile) and a custom file directory (strOutputDir) for clarity and ease of use for the user (i think this addresses #1755).

I'm also considering addressing #1724 with this PR as well, since it relates to @dpastoor's point about specifying output directories and file names within workflows.

We can discuss further in the morning- I'll be logging off shortly.

[dpastoor]

Thanks @lauramaxwell for the thoughts, I like some of the ideas you've proposed here! unfortunately I do not think this will fix the issue I was running into.

In particular, the issue I was running would still hit here I think:

report_path <- system.file("report", "Report_KRI.Rmd", package = "gsm")

when the gsm package dir is read only, the resulting render will fail, because it drops an intermediate md file in the directory of the rmd, so from an access perspective, instead if we could change the file.access check to instead by that if the report_path is a read-only dir, then we'd want to shift it to a temp dir.

My other general concern with the approach of rendering within the packages system.file/inst dir has other potential problems - if you try to run a Report_KRI from two processes at the same time they can clobber each other with that intermediate file dropped. It has always felt safest to me that when you're rendering in any sort of scenario that there could be any concurrent access to where a render is called, to do that in a temp dir scoped to the process.

[lauramaxwell]

ah, right. Thanks, @dpastoor. I got a bit mixed up trying to solve for both issues. I think i have a solution-

1. always create a temp directory for the intermediary files
2. always use an absolute path for the output directory (default to user's working directory)
3. if working directory (or supplied strOutputDir) is not writable, then send a message to the user and use the temp directory to save the final html report

Does this seem like it will solve the issue?

[lauramaxwell]

If you're like me, i like to see the code- so something like this:

 # run report in temp directory
  tpath <- fs::path_temp()
  report_path <- file.path(tpath, "Report_KRI.Rmd")
  fs::file_copy(system.file("report", "Report_KRI.Rmd", package = "gsm"), report_path)
  # currently report_kri also needs a styles.css dep
  fs::file_copy(system.file("report", "styles.css", package = "gsm"), file.path(tpath, "styles.css"))

  # specify strOutputDir path, depending on write access to strOutputDir
  if (file.access(strOutputDir, mode = 2) == -1) {
    cli::cli_inform("You do not have permission to write to {strOutputDir}. Report will be saved to {tpath}")
    strOutputDir <- tpath
  }

  # set output file absolute path
  if (is.null(strOutputFile)) {
    GroupLevel <- unique(dfMetrics$GroupLevel)
    StudyID <- unique(dfResults$StudyID)
    SnapshotDate <- max(unique(dfResults$SnapshotDate))
    if (length(GroupLevel == 1) & length(StudyID) == 1) {
      # remove non alpha-numeric characters from StudyID, GroupLevel and SnapshotDate
      StudyID <- gsub("[^[:alnum:]]", "", StudyID)
      GroupLevel <- gsub("[^[:alnum:]]", "", GroupLevel)
      SnapshotDate <- gsub("[^[:alnum:]]", "", as.character(SnapshotDate))

      strOutpath <- file.path(strOutputDir, paste0("kri_report_", StudyID, "_", GroupLevel, "_", SnapshotDate, ".html"))
    } else {
      strOutpath <- file.path(strOutputDir, "kri_report.html")
    }
  } else {
    strOutpath <- file.path(strOutputDir, strOutputFile)
  }

  rmarkdown::render(
    report_path,
    output_file = strOutpath,
    params = list(
      lCharts = lCharts,
      dfResults = dfResults,
      dfGroups = dfGroups,
      dfMetrics = dfMetrics
    ),
    envir = new.env(parent = globalenv())
  )

[jonthegeek]

ah, right. Thanks, @dpastoor. I got a bit mixed up trying to solve for both issues. I think i have a solution-

1. always create a temp directory for the intermediary files
2. always use an absolute path for the output directory (default to user's working directory)
3. if working directory (or supplied strOutputDir) is not writable, then send a message to the user and use the temp directory to save the final html report

Does this seem like it will solve the issue?

Good call! I'd also update the examples (and any other usage) to assign the function call to something, like kri_report_path <- Report_KRI(...), because we're returning the actual path invisibly; it would be good to demonstrate that. Oh, and also update the @return. I'll put this stuff into a review once you're ready for one!

[lauramaxwell]

ok, i'm moving forward with this. it also looks like we can specify intermediate_dir and knit_root_dir in rmarkdown::render() so, we should be able to skip the file_copy steps above and just change those arguments to the temp directory.

[lauramaxwell]

@jonthegeek @dpastoor this is ready for re-review. I think we can work on #1724 in a separate PR, unless one of you wants to tackle it here.

[jonthegeek]

Since we're exporting RenderRmd, I made some suggestions to harden it a bit. Let's do all the checks there, since anything we save will need to deal with the write access thing. Also be sure to update anywhere that these functions are called (I didn't check that the args all still make sense).

[lauramaxwell]

@jonthegeek edits made! thanks for the feedback :)

[jonthegeek]

Looks good!