From 2e22e59fa0b43d62d815c0f8effba55a2af4663e Mon Sep 17 00:00:00 2001 From: Andrie de Vries Date: Mon, 29 May 2017 16:41:29 +0100 Subject: [PATCH] Refactor code, fix unit tests #62 --- R/AzureContainer.R | 1 + R/AzureContextObject.R | 50 ++++++++++++++++++++--------- R/AzureResources.R | 4 +-- R/AzureStorageAccount.R | 46 +++++++++------------------ R/AzureTemplate.R | 11 ++++--- R/internal.R | 2 ++ R/methods.R | 2 +- tests/testthat.R | 7 +++-- tests/testthat/test-2-resources.R | 13 ++++---- tests/testthat/test-3-vm.R | 9 ++++-- tests/testthat/test-4-scalesets.R | 52 +++++++++++++++---------------- 11 files changed, 107 insertions(+), 90 deletions(-) diff --git a/R/AzureContainer.R b/R/AzureContainer.R index c6c4a7c..f4e4975 100644 --- a/R/AzureContainer.R +++ b/R/AzureContainer.R @@ -28,6 +28,7 @@ azureListStorageContainers <- function(azureActiveContext, storageAccount, stora verbosity <- set_verbosity(verbose) URL <- paste0("http://", storageAccount, ".blob.core.windows.net/?comp=list") + #browser() xdate <- x_ms_date() SIG <- getSig(azureActiveContext, url = URL, verb = "GET", key = storageKey, storageAccount = storageAccount, CMD = "\ncomp:list", date = xdate) diff --git a/R/AzureContextObject.R b/R/AzureContextObject.R index 9ccea28..781e5a7 100644 --- a/R/AzureContextObject.R +++ b/R/AzureContextObject.R @@ -32,22 +32,21 @@ createAzureContext <- function(tenantID, clientID, authKey){ #' @param clientID The Client ID provided during creation of the Active Directory application / service principal #' @param authKey The Authentication Key provided during creation of the Active Directory application / service principal #' @param subscriptionID Subscription ID. This is obtained automatically by [azureAuthenticate()] when only a single subscriptionID is available via Active Directory -#' @param azToken Azure authentication token, obtained by [azureAuthenticate()] #' @param resourceGroup Name of the resource group #' @param vmName Name of the virtual machine -#' @param storageAccount Name of the azure storage account +#' @param storageAccount Name of the azure storage account. Storage account names must be between 3 and 24 characters in length and use numbers and lower-case letters only. #' @param storageKey Storage key associated with storage account #' @param blob Blob name #' @param clustername Cluster name, used for HDI and Spark clusters. See [azureCreateHDI()] #' @param sessionID Spark sessionID. See [azureSparkCMD()] -#' @param hdiAdmin HDInsight admin username -#' @param hdiPassword HDInsight admin password +#' @param hdiAdmin HDInsight admin username. See [azureCreateHDI()] +#' @param hdiPassword HDInsight admin password. See [azureCreateHDI()] #' @param container Storage container name. See [azureListStorageContainers()] -#' @param kind HDinsight kind: "hadoop","spark" or "pyspark" +#' @param kind HDinsight kind: "hadoop","spark" or "rserver". See [azureCreateHDI()] #' #' @family azureActiveContext functions #' @export -setAzureContext <- function(azureActiveContext, tenantID, clientID, authKey, azToken, +setAzureContext <- function(azureActiveContext, tenantID, clientID, authKey, subscriptionID, resourceGroup, storageKey, storageAccount, container, blob, @@ -57,14 +56,37 @@ setAzureContext <- function(azureActiveContext, tenantID, clientID, authKey, azT if (!missing(tenantID)) azureActiveContext$tenantID <- tenantID if (!missing(clientID)) azureActiveContext$clientID <- clientID if (!missing(authKey)) azureActiveContext$authKey <- authKey - if (!missing(azToken)) azureActiveContext$AZtoken <- azToken - if (!missing(subscriptionID)) azureActiveContext$subscriptionID <- subscriptionID - if (!missing(resourceGroup)) azureActiveContext$resourceGroup <- resourceGroup - if (!missing(storageKey)) azureActiveContext$storageKey <- storageKey - if (!missing(storageAccount)) azureActiveContext$storageAccount <- storageAccount - if (!missing(container)) azureActiveContext$container <- container - if (!missing(blob)) azureActiveContext$container <- blob - if (!missing(vmName)) azureActiveContext$vmName <- vmName + + if (!missing(subscriptionID)) { + assert_that(is_subscription_id(subscriptionID)) + azureActiveContext$subscriptionID <- subscriptionID + } + if (!missing(resourceGroup)) { + assert_that(is_resource_group(resourceGroup)) + azureActiveContext$resourceGroup <- resourceGroup + } + if (!missing(storageKey)) { + assert_that(is_storage_key(storageKey)) + azureActiveContext$storageKey <- storageKey + } + if (!missing(storageAccount)) { + assert_that(is_storage_account(storageAccount)) + azureActiveContext$storageAccount <- storageAccount + } + if (!missing(container)) { + assert_that(is_container(container)) + azureActiveContext$container <- container + } + if (!missing(blob)) { + assert_that(is_blob(blob)) + azureActiveContext$container <- blob + } + + if (!missing(vmName)) { + assert_that(is_vm_name(vmName)) + azureActiveContext$vmName <- vmName + } + if (!missing(clustername)) azureActiveContext$clustername <- clustername if (!missing(hdiAdmin)) azureActiveContext$hdiAdmin <- hdiAdmin if (!missing(hdiPassword)) azureActiveContext$hdiPassword <- hdiPassword diff --git a/R/AzureResources.R b/R/AzureResources.R index 48c36be..c08359d 100644 --- a/R/AzureResources.R +++ b/R/AzureResources.R @@ -18,8 +18,8 @@ azureListSubscriptions <- function(azureActiveContext, verbose = FALSE) { stopWithAzureError(r) dfs <- lapply(content(r), data.frame, stringsAsFactors = FALSE) - df1 <- rbind.fill(dfs) - if (nrow(df1) == 1) azureActiveContext$subscriptionID <- df1[, 2] + df1 <- do.call(rbind, dfs)) + if (nrow(df1) == 1) azureActiveContext$subscriptionID <- df1$subscriptionID[1] return(df1) } diff --git a/R/AzureStorageAccount.R b/R/AzureStorageAccount.R index a3cd340..47455a9 100644 --- a/R/AzureStorageAccount.R +++ b/R/AzureStorageAccount.R @@ -34,7 +34,6 @@ azureListSA <- function(azureActiveContext, resourceGroup, subscriptionID, #' #' @inheritParams setAzureContext #' @inheritParams azureAuthenticate -#' @param storageAccount storageAccount #' #' @family Storage account functions #' @export @@ -98,13 +97,12 @@ azureCreateStorageAccount <- function(azureActiveContext, storageAccount, verbosity <- set_verbosity(verbose) - bodyI <- '{ - "location": "llllllll", + bodyI <- paste0('{ + "location":"', location, '", "sku": { - "name": "Standard_LRS" + "name": "Standard_LRS" }}' - - bodyI <- gsub("llllllll", location, bodyI) + ) URL <- paste0("https://management.azure.com/subscriptions/", subscriptionID, "/resourceGroups/", resourceGroup, "/providers/Microsoft.Storage/storageAccounts/", @@ -127,7 +125,7 @@ azureCreateStorageAccount <- function(azureActiveContext, storageAccount, azureActiveContext$resourceGroup <- resourceGroup message("Create request Accepted. It can take a few moments to provision the storage account") - if (!asynchronous) {a + if (!asynchronous) { wait_for_azure( storageAccount %in% azureListSA(azureActiveContext)$storageAccount ) @@ -146,41 +144,27 @@ azureCreateStorageAccount <- function(azureActiveContext, storageAccount, #' @export azureDeletestorageAccount <- function(azureActiveContext, storageAccount, resourceGroup, subscriptionID, verbose = FALSE) { + assert_that(is.azureActiveContext(azureActiveContext)) azureCheckToken(azureActiveContext) + azToken <- azureActiveContext$Token - if (missing(resourceGroup)) { - resourceGroup <- azureActiveContext$resourceGroup - } + if (missing(resourceGroup)) resourceGroup <- azureActiveContext$resourceGroup + if (missing(subscriptionID)) subscriptionID <- azureActiveContext$subscriptionID - if (missing(subscriptionID)) { - subscriptionID <- azureActiveContext$subscriptionID - } - if (!length(storageAccount)) { - stop("Error: No Valid storageAccount provided") - } - if (length(resourceGroup) < 1) { - stop("Error: No resourceGroup provided: Use resourceGroup argument or set in AzureContext") - } - if (!length(subscriptionID)) { - stop("Error: No subscriptionID provided: Use SUBID argument or set in AzureContext") - } + assert_that(is_storage_account(storageAccount)) + assert_that(is_resource_group(resourceGroup)) + assert_that(is_subscription_id(subscriptionID)) verbosity <- set_verbosity(verbose) - URL <- paste0("https://management.azure.com/subscriptions/", subscriptionID, "/resourceGroups/", resourceGroup, "/providers/Microsoft.Storage/storageAccounts/", storageAccount, "?api-version=2016-01-01") - r <- DELETE(URL, add_headers(.headers = c(Host = "management.azure.com", - Authorization = azureActiveContext$Token, - `Content-type` = "application/json")), - verbosity) + r <- DELETE(URL, azureApiHeaders(azToken), verbosity) if (status_code(r) == 204) { - stop("Error: Storage Account not found") - } - if (status_code(r) == 409) { - stop("Error: An operation for the storage account is in progress.") + warning("Storage Account not found") + return(FALSE) } if (status_code(r) != 200) stopWithAzureError(r) diff --git a/R/AzureTemplate.R b/R/AzureTemplate.R index 7800a59..70d9b7b 100644 --- a/R/AzureTemplate.R +++ b/R/AzureTemplate.R @@ -15,7 +15,7 @@ #' @family Template functions #' @export azureDeployTemplate <- function(azureActiveContext, deplname, templateURL, - paramURL, templateJSON, paramJSON, mode = "Sync", + paramURL, templateJSON, paramJSON, mode = c("Sync", "Async"), resourceGroup, subscriptionID, verbose = FALSE) { assert_that(is.azureActiveContext(azureActiveContext)) @@ -28,6 +28,8 @@ azureDeployTemplate <- function(azureActiveContext, deplname, templateURL, assert_that(is_subscription_id(subscriptionID)) assert_that(is_deployment_name(deplname)) + mode <- match.arg(mode) + if (missing(templateURL) && missing(templateJSON)) { stop("No templateURL or templateJSON provided") } @@ -35,8 +37,9 @@ azureDeployTemplate <- function(azureActiveContext, deplname, templateURL, verbosity <- set_verbosity(verbose) URL <- paste0("https://management.azure.com/subscriptions/", subscriptionID, - "/resourceGroups/", resourceGroup, "/providers/microsoft.resources/deployments/", - deplname, "?api-version=2016-06-01") + "/resourceGroups/", resourceGroup, + "/providers/microsoft.resources/deployments/", deplname, + "?api-version=2016-06-01") combination <- paste0(if(!missing(templateURL)) "tu" else "tj" , if (!missing(paramURL)) "pu" else if (!missing(paramJSON)) "pj" else "") @@ -64,7 +67,7 @@ azureDeployTemplate <- function(azureActiveContext, deplname, templateURL, r <- PUT(URL, azureApiHeaders(azToken), body = bodyI, verbosity) stopWithAzureError(r) - if (toupper(mode) == "SYNC") { + if (mode == "Sync") { z <- pollStatusTemplate(azureActiveContext, deplname, resourceGroup) if(!z) return(FALSE) } diff --git a/R/internal.R b/R/internal.R index a47d97f..82b11a9 100644 --- a/R/internal.R +++ b/R/internal.R @@ -145,6 +145,7 @@ stopWithAzureError <- function(r) { rr <- XML::xmlToList(XML::xmlParse(content(r))) msg <- addToMsg(rr$Code) msg <- addToMsg(rr$Message) + msg <- addToMsg(rr$AuthenticationErrorDetail) } else { rr <- content(r) msg <- addToMsg(rr$code) @@ -160,6 +161,7 @@ extractResourceGroupname <- function(x) gsub(".*?/resourceGroups/(.*?)(/.*)*$", extractSubscriptionID <- function(x) gsub(".*?/subscriptions/(.*?)(/.*)*$", "\\1", x) extractStorageAccount <- function(x) gsub(".*?/storageAccounts/(.*?)(/.*)*$", "\\1", x) + refreshStorageKey <- function(azureActiveContext, storageAccount, resourceGroup){ if (length(azureActiveContext$storageAccountK) < 1 || storageAccount != azureActiveContext$storageAccountK || diff --git a/R/methods.R b/R/methods.R index 4fa3dfe..b6f0d28 100644 --- a/R/methods.R +++ b/R/methods.R @@ -128,7 +128,7 @@ is_valid_storage_account <- function(x) { on_failure(is_valid_storage_account) <- function(call, env) { paste("Storage account name must be between 3 and 24 characters in length", - "and use numbers and lower - case letters only.", + "and use numbers and lower-case letters only.", sep = "\n") } diff --git a/tests/testthat.R b/tests/testthat.R index f10ff42..765d8c8 100644 --- a/tests/testthat.R +++ b/tests/testthat.R @@ -3,12 +3,15 @@ library(testthat, quietly = TRUE) if (identical(Sys.getenv("NOT_CRAN"), "true")) { # NOT_CRAN # run all tests + test_check("AzureSMR") + # test_check("AzureSMR", filter = "1-authentication") + # test_check("AzureSMR", filter = "2-resources") } else { # CRAN # skip some tests on CRAN, to comply with timing directive and other policy test_check("AzureSMR") - # test_check("AzureSMR", filter = "1-workspace-no-config") - # test_check("AzureSMR", filter = "7-discover-schema") + # test_check("AzureSMR", filter = "1-authentication") + # test_check("AzureSMR", filter = "2-resources") } diff --git a/tests/testthat/test-2-resources.R b/tests/testthat/test-2-resources.R index d309a3a..5aa047f 100644 --- a/tests/testthat/test-2-resources.R +++ b/tests/testthat/test-2-resources.R @@ -63,10 +63,6 @@ test_that("Can connect to storage account", { expect_is(res, "data.frame") expect_equal(ncol(res), 8) - #sub_id <<- res$storageAccount[1] - #rg_temp <<- res$resourceGroup[1] - #res <- azureSAGetKey(asc, storageAccount = sub_id, resourceGroup = rg_temp) - #expect_is(res, "character") }) test_that("Can create storage account", { @@ -86,9 +82,12 @@ test_that("Can create storage account", { context(" - container") test_that("Can connect to container", { skip_if_missing_config(settingsfile) - sa <- azureListSA(asc)[1, ] - res <- azureListStorageContainers(asc, storageAccount = sa$storageAccount[1], - resourceGroup = sa$resourceGroup[1]) + sa <- azureListSA(asc) + idx <- match(sa_name, sa$storageAccount) + key <- storageKey <- azureSAGetKey(asc, resourceGroup = sa$resourceGroup[idx], storageAccount = sa$storageAccount[idx]) + res <- azureListStorageContainers(asc, storageAccount = sa$storageAccount[idx], + resourceGroup = sa$resourceGroup[idx], + storageKey = key) expect_is(res, "data.frame") expect_equal(ncol(res), 5) expect_equal(nrow(res), 0) diff --git a/tests/testthat/test-3-vm.R b/tests/testthat/test-3-vm.R index 7ea7f81..ff6d50d 100644 --- a/tests/testthat/test-3-vm.R +++ b/tests/testthat/test-3-vm.R @@ -41,12 +41,15 @@ test_that("Can create virtual machine from template", { azureTemplatesUrl <- "https://raw.githubusercontent.com/Azure/azure-quickstart-templates/master/" templateURL = paste0(azureTemplatesUrl, "101-vm-simple-linux/azuredeploy.json") - paramJSON <- '"parameters": { + paramJSON <- paste0('"parameters": { "adminUsername": {"value": "azuresmr"}, "adminPassword": {"value": "Azuresmrtest123!"}, - "dnsLabelPrefix": {"value": "azuresmr"}}' + "dnsLabelPrefix": {"value": "', paste0("azuresmr", timestamp), '"}}' + ) - res <- azureDeployTemplate(asc, deplname = "Deploy2", templateURL = templateURL, paramJSON = paramJSON) + res <- azureDeployTemplate(asc, deplname = "Deploy2", + templateURL = templateURL, + paramJSON = paramJSON) expect_true(res) }) diff --git a/tests/testthat/test-4-scalesets.R b/tests/testthat/test-4-scalesets.R index 485e777..952568d 100644 --- a/tests/testthat/test-4-scalesets.R +++ b/tests/testthat/test-4-scalesets.R @@ -1,41 +1,41 @@ -if (interactive()) library("testthat") +#if (interactive()) library("testthat") -settingsfile <- system.file("tests/testthat/config.json", package = "AzureSMR") -config <- read.AzureSMR.config(settingsfile) +#settingsfile <- system.file("tests/testthat/config.json", package = "AzureSMR") +#config <- read.AzureSMR.config(settingsfile) -# ------------------------------------------------------------------------ +## ------------------------------------------------------------------------ -context("Scalesets") +#context("Scalesets") -asc <- createAzureContext() -with(config, - setAzureContext(asc, tenantID = tenantID, clientID = clientID, authKey = authKey) -) +#asc <- createAzureContext() +#with(config, + #setAzureContext(asc, tenantID = tenantID, clientID = clientID, authKey = authKey) +#) -azureAuthenticate(asc, verbose = FALSE) +#azureAuthenticate(asc, verbose = FALSE) -timestamp <- format(Sys.time(), format = "%y%m%d%H%M") -resourceGroup_name <- paste0("_AzureSMtest_", timestamp) +#timestamp <- format(Sys.time(), format = "%y%m%d%H%M") +#resourceGroup_name <- paste0("_AzureSMtest_", timestamp) -#test_that("Can create resource group", { - #skip_if_missing_config(settingsfile) +##test_that("Can create resource group", { + ##skip_if_missing_config(settingsfile) - #res <- azureCreateResourceGroup(asc, location = "westeurope", resourceGroup = resourceGroup_name) - #expect_true(res) + ##res <- azureCreateResourceGroup(asc, location = "westeurope", resourceGroup = resourceGroup_name) + ##expect_true(res) - #wait_for_azure( - #resourceGroup_name %in% azureListRG(asc)$resourceGroup - #) - #expect_true(resourceGroup_name %in% azureListRG(asc)$resourceGroup) -#}) + ##wait_for_azure( + ##resourceGroup_name %in% azureListRG(asc)$resourceGroup + ##) + ##expect_true(resourceGroup_name %in% azureListRG(asc)$resourceGroup) +##}) -azureCreateHDI(asc) +#azureCreateHDI(asc) -azureListScaleSets(asc) -azureListScaleSetNetwork(asc) -azureListScaleSetVM(asc, scaleSet = "swarm-agent-D2D9AE69-vmss", resourceGroup = "THDELTEI-TEST-CONTAINER2") -azureListScaleSetVM(asc, scaleSet = "swarm-agent-D2D9AE69-vmss") +#azureListScaleSets(asc) +#azureListScaleSetNetwork(asc) +#azureListScaleSetVM(asc, scaleSet = "swarm-agent-D2D9AE69-vmss", resourceGroup = "THDELTEI-TEST-CONTAINER2") +#azureListScaleSetVM(asc, scaleSet = "swarm-agent-D2D9AE69-vmss")