New feature: System diagnostics functions #98

rmbielby · 2024-11-11T16:34:49Z

Brief overview of changes

I've added a master diagnostics_test() function to sit as a master diagnostics function to run any diagnostic scripts we add. So far it encompasses the following two functions that I've also added as part of this PR:

check_proxy_settings() - this looks for (and removes if clean=TRUE) any rogue proxy settings in the Git configuration
check_github_pat() - this looks for (and removes if clean=TRUE) the system variable GITHUB_PAT, which prevents analysts from installing packages from GitHub
check_renv_download_method() - checks if the renv_download_method has been set in the .Renviron file and flags if either a) it's not set, or b) it's set to something other than "curl". If run with clean=TRUE, then it removes any none-curl setting and adds a line to .Renviron setting the method to curl.

Why are these changes being made?

Analysts sometimes hit issues with settings on their laptop (usually that they've set themselves after following outdated guidance) and find it difficult to diagnose and fix these issues themselves. We need a way for them to do this in an automated way.

Detailed description of changes

All the extra code is self contained in the diagnostic_test.R script:

diagnostic_test()
check_proxy_settings()
check_github_pat()
check_renv_download_method()

The functions have some consistent params:

clean = TRUE / FALSE - this dictates whether the function should automatically clean up any issues it finds. Default behaviour is just to output messages to the console about the issues.
verbose = TRUE / FALSE - adds a little bit of extra info

I've also added an override parameter for each function, that mainly just helps with testing, but could conceivably be used by us to deal with unusual cases, e.g. you can set the config keys to search for and remove in clean_proxy_settings() if there's a key other than http.proxy and https.proxy that's causing problems.

I've added tests for the individual check_* functions. Except for check_github_pat() as I couldn't find a way to identify the contents of Sys.getenv("GITHUB_PAT") when running on GitHub Actions. It seems to return something that looks like a string "***", but doesn't act like one (i.e. I can't do logic on it and str_replace_all("*", "") doesn't reduce it to a blank string). Main thought is that it's acting as some sort of secret / encrypted variable, but couldn't find anything remotely equivalent from searching online. The test that I had worked fine locally, but just won't work equivalently on GitHub Actions.

Additional information for reviewers

Issue ticket number/s and link

#63

…f checks output

…GitHub Actions

…ironment variable to the test script

cjrace

Initial pass with a few comments - not sure it's all quite working as it should?

cjrace · 2024-11-18T13:41:11Z

R/diagnostic_test.R

+  proxy_config <- git_config |>
+    magrittr::extract2("global") |>
+    magrittr::extract(proxy_setting_names)
+  proxy_config <- purrr::keep(proxy_config, !is.na(names(proxy_config)))


I think you might be able to remove the need for purrr entirely if you swap this line out, this might work as an alternative (though you should check it does actually do what you intended)

proxy_config <- proxy_config[!is.na(names(proxy_config))]

cjrace · 2024-12-02T13:02:17Z

R/diagnostic_test.R

+      message("FAIL: Git proxy setting have been left in place.")
+    }
+  } else {
+    message("PASS: No proxy settings found in your Git configuration.")


Currently when passing it prints everything whether verbose or not, should it just print the pass message if verbose = FALSE (default)?

With verbose = FALSE, still shows all information

cjrace · 2024-12-02T13:12:16Z

R/diagnostic_test.R

+#' @description
+#' This script checks for "bad" proxy settings. Prior to the pandemic, analysts
+#' in the DfE would need to add some proxy settings to their GitHub config.
+#' These settings now prevent Git from connecting to remote archives on GitHub
+#' and Azure DevOps if present, so this script identifies and (if clean=TRUE is
+#' set) removes them.


The old code (3233fe1#diff-9152ef3c45461a2c8473510069ec582ac76fa432c711c8d9279d32dcceaa2b98L21) set http_proxy and https_proxy as environment variables - I think this function should be checking for those too using Sys.getenv()?

cjrace · 2024-12-02T13:14:26Z

R/diagnostic_test.R

+  results <- c(
+    check_proxy_settings(clean = clean, verbose = verbose),
+    check_github_pat(clean = clean, verbose = verbose),
+    check_renv_download_method(clean = clean, verbose = verbose)


Could we add an additional check for http.sslVerify in their Git config too?

system("git config --global http.sslVerify false") - was a line in the original proxy script and we should be discouraging analysts from having it in their configs anymore (as it's not longer necessary)

cjrace · 2024-12-02T13:16:05Z

R/diagnostic_test.R

+#'
+#' @examples
+#' check_proxy_settings()
+check_proxy_settings <- function(


Doesn't seem to be working, I've set http.proxy in my Git config

But running the function even after restarting R Studio doesn't seem to pick it up? Am I doing something wrong here?

cjrace · 2024-12-02T13:17:15Z

R/diagnostic_test.R

+  github_pat <- Sys.getenv("GITHUB_PAT")
+  # Replace above to remove non alphanumeric characters when run on GitHub
+  # Actions
+  cat("==================================")


Needs to be message() or have a \n for a new line at the end

cjrace · 2024-12-02T13:19:26Z

R/diagnostic_test.R

+  # Replace above to remove non alphanumeric characters when run on GitHub
+  # Actions
+  cat("==================================")
+  if (!is.na(github_pat)) {


I think this is giving false positives, my GitHub PAT is unset but it's flagging as a fail. Should treat "" as the equivalent of NA?

To illustrate it, even a made up variable I can guarantee has never been set on my machine returns as blank quotes rather than NA

cjrace · 2024-12-02T13:20:45Z

R/diagnostic_test.R

+check_renv_download_method <- function(
+    renviron_file = "~/.Renviron",
+    clean = FALSE,
+    verbose = FALSE) {


Feel like the verbose = FALSE option should only print the success message, feels a bit too verbose printing the variable as well as the message?

cjrace · 2024-12-02T13:24:08Z

tests/testthat/test-diagnostic_test.R

@@ -0,0 +1,44 @@
+test_that("Check proxy settings identifies and removes proxy setting", {
+  # Set a dummy config parameter for the purposes of testing
+  git2r::config(http.proxy.test = "this-is-a-test-entry", global = TRUE)


I'm wondering if these tests are passing because the config isn't being set?

Could be worth adding an extra step in just to make sure the config is being set prior to being removed to give us more confidence in them?

I've just used this and it doesn't appear to have set anything when I run git config -l in a terminal?

cjrace · 2024-12-02T13:24:57Z

tests/testthat/test-diagnostic_test.R

+})
+
+test_that("Check RENV_DOWNLOAD_METHOD", {
+  # Check that check_proxy_settings identifies the rogue entry


What's the rogue entry it's looking for?

rmbielby added 8 commits November 8, 2024 15:38

Created initial diagnostics script with proxy checking function

195c980

Added RENV_DOWNLOAD_METHOD diagnostic script

60a349e

Simplified diagnostic test workflows

f02301f

Lintr and code styling

3d23dfd

More lintr and code styling

e18e029

Added some description to diagnostic scripts

faa6a6e

Added a test for the renv download method diagnostic check

f029e76

Updating diagnostics documentation

89a7d4c

rmbielby added the enhancement New feature or request label Nov 11, 2024

rmbielby requested a review from cjrace November 11, 2024 16:34

rmbielby self-assigned this Nov 11, 2024

rmbielby linked an issue Nov 11, 2024 that may be closed by this pull request

Add a dfe-specific diagnostics function #63

Open

Added GITHUB_PAT check to diagnostic tools

42e0600

rmbielby linked an issue Nov 12, 2024 that may be closed by this pull request

Add a post proxy.R clean up function #56

Open

rmbielby added 11 commits November 12, 2024 14:22

Adapted proxy settings check output to be more consistent with rest o…

03f0437

…f checks output

Cleaning up and updating pkgdown with check github_pat

0d1e2f2

Accounting for GitHub Actions GITHUB_PAT system variable of ***

1853b75

Switching messages on for GITHUB_PAT check to help diagnose issue on …

17ed957

…GitHub Actions

Switching messages on for GITHUB_PAT check to help diagnose issue on …

f1c7bee

…GitHub Actions

Switching messages on for GITHUB_PAT check to help diagnose issue on …

545fe68

…GitHub Actions

Switching messages on for GITHUB_PAT check to help diagnose issue on …

1970f0f

…GitHub Actions

Switching messages on for GITHUB_PAT check to help diagnose issue on …

4fb6e61

…GitHub Actions

Switching messages on for GITHUB_PAT check to help diagnose issue on …

fd8d896

…GitHub Actions

Removing GITHUB_PAT check test as GitHub actions won't return the env…

1ea5261

…ironment variable to the test script

Code styling

ec631aa

cjrace requested changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: System diagnostics functions #98

New feature: System diagnostics functions #98

rmbielby commented Nov 11, 2024 •

edited

Loading

cjrace left a comment

cjrace Nov 18, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

cjrace Dec 2, 2024

New feature: System diagnostics functions #98

Are you sure you want to change the base?

New feature: System diagnostics functions #98

Conversation

rmbielby commented Nov 11, 2024 • edited Loading

Brief overview of changes

Why are these changes being made?

Detailed description of changes

Additional information for reviewers

Issue ticket number/s and link

cjrace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmbielby commented Nov 11, 2024 •

edited

Loading