Refactor code, fix unit tests #62

microsoft · May 29, 2017 · 2e22e59 · 2e22e59
1 parent e4eebbf
commit 2e22e59
Show file tree

Hide file tree

Showing 11 changed files with 107 additions and 90 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,15 +28,18 @@ 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")
   }
 
   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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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")