Merge pull request #64 from OctopusDeploy/fix-http-404-variables

fix: implementation for variable service to return 404s (#63)

integration/variable_service_test.go

@@ -0,0 +1,164 @@
+package integration
+
+import (
+	"testing"
+
+	"github.com/OctopusDeploy/go-octopusdeploy/octopusdeploy"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func getTestVariable(name string) *octopusdeploy.Variable {
+	variable := octopusdeploy.NewVariable(name)
+	variable.Description = "octo-test description"
+	variable.Value = "octo-test value"
+
+	return variable
+}
+
+func CreateTestVariable(t *testing.T, ownerID string, name string) *octopusdeploy.Variable {
+	client := getOctopusClient()
+	require.NotNil(t, client)
+
+	variable := getTestVariable(name)
+	variableSet, err := client.Variables.AddSingle(ownerID, variable)
+	require.NoError(t, err)
+	require.Len(t, variableSet.Variables, 1)
+
+	for _, v := range variableSet.Variables {
+		if v.Name == name {
+			createdVariable, err := client.Variables.GetByID(ownerID, v.GetID())
+			require.NoError(t, err)
+			require.NotNil(t, createdVariable)
+
+			return createdVariable
+		}
+	}
+
+	return nil
+}
+
+func DeleteTestVariable(t *testing.T, octopusClient *octopusdeploy.Client, variableID string, ownerID string) {
+	if octopusClient == nil {
+		octopusClient = getOctopusClient()
+	}
+	require.NotNil(t, octopusClient)
+
+	variableSet, err := octopusClient.Variables.DeleteSingle(ownerID, variableID)
+	assert.NoError(t, err)
+	assert.NotNil(t, variableSet)
+}
+
+func TestVariableService(t *testing.T) {
+	client := getOctopusClient()
+	require.NotNil(t, client)
+
+	lifecycle := CreateTestLifecycle(t, client)
+	require.NotNil(t, lifecycle)
+	defer DeleteTestLifecycle(t, client, lifecycle)
+
+	projectGroup := CreateTestProjectGroup(t, client)
+	require.NotNil(t, projectGroup)
+	defer DeleteTestProjectGroup(t, client, projectGroup)
+
+	project := CreateTestProject(t, client, lifecycle, projectGroup)
+	require.NotNil(t, project)
+	defer DeleteTestProject(t, client, project)
+
+	name := getRandomVarName()
+	expected := getTestVariable(name)
+	createdVariable := CreateTestVariable(t, project.GetID(), name)
+	defer DeleteTestVariable(t, client, createdVariable.GetID(), project.GetID())
+
+	require.Equal(t, expected.Name, createdVariable.Name, "variable name doesn't match expected")
+	require.NotEmpty(t, createdVariable.GetID(), "variable doesn't contain an ID from the octopus server")
+
+	variable, err := client.Variables.GetByID(project.GetID(), createdVariable.GetID())
+	require.NoError(t, err)
+	require.Equal(t, expected.Name, variable.Name, "variable name doesn't match expected")
+	require.NotEmpty(t, variable.GetID(), "variable doesn't contain an ID from the octopus server")
+}
+
+func TestVariableServiceGetByID(t *testing.T) {
+	client := getOctopusClient()
+	require.NotNil(t, client)
+
+	ownerID := getShortRandomName()
+	variableID := getShortRandomName()
+
+	variable, err := client.Variables.GetByID(ownerID, variableID)
+	require.Error(t, err)
+	require.Nil(t, variable)
+
+	apiError := err.(*octopusdeploy.APIError)
+	require.Equal(t, apiError.StatusCode, 404)
+
+	lifecycle := CreateTestLifecycle(t, client)
+	require.NotNil(t, lifecycle)
+	defer DeleteTestLifecycle(t, client, lifecycle)
+
+	projectGroup := CreateTestProjectGroup(t, client)
+	require.NotNil(t, projectGroup)
+	defer DeleteTestProjectGroup(t, client, projectGroup)
+
+	project := CreateTestProject(t, client, lifecycle, projectGroup)
+	require.NotNil(t, project)
+	defer DeleteTestProject(t, client, project)
+
+	variable, err = client.Variables.GetByID(project.GetID(), variableID)
+	require.Error(t, err)
+	require.Nil(t, variable)
+
+	apiError = err.(*octopusdeploy.APIError)
+	require.Equal(t, apiError.StatusCode, 404)
+}
+
+func TestVariableServiceDeleteSingle(t *testing.T) {
+	client := getOctopusClient()
+	require.NotNil(t, client)
+
+	ownerID := getShortRandomName()
+	variableID := getShortRandomName()
+
+	variableSet, err := client.Variables.DeleteSingle(ownerID, variableID)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+
+	apiError := err.(*octopusdeploy.APIError)
+	require.Equal(t, apiError.StatusCode, 404)
+
+	lifecycle := CreateTestLifecycle(t, client)
+	require.NotNil(t, lifecycle)
+	defer DeleteTestLifecycle(t, client, lifecycle)
+
+	projectGroup := CreateTestProjectGroup(t, client)
+	require.NotNil(t, projectGroup)
+	defer DeleteTestProjectGroup(t, client, projectGroup)
+
+	project := CreateTestProject(t, client, lifecycle, projectGroup)
+	require.NotNil(t, project)
+	defer DeleteTestProject(t, client, project)
+
+	variableSet, err = client.Variables.DeleteSingle(project.GetID(), variableID)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+
+	apiError = err.(*octopusdeploy.APIError)
+	require.Equal(t, apiError.StatusCode, 404)
+
+	expectedVariableSet, err := client.Variables.GetAll(project.GetID())
+	require.NoError(t, err)
+	require.NotNil(t, expectedVariableSet.ScopeValues)
+
+	name := getRandomVarName()
+	expected := getTestVariable(name)
+	createdVariable := CreateTestVariable(t, project.GetID(), name)
+
+	require.Equal(t, expected.Name, createdVariable.Name, "variable name doesn't match expected")
+	require.NotEmpty(t, createdVariable.GetID(), "variable doesn't contain an ID from the octopus server")
+
+	variableSet, err = client.Variables.DeleteSingle(project.GetID(), createdVariable.GetID())
+	require.NoError(t, err)
+	require.NotNil(t, variableSet)
+	require.Equal(t, expectedVariableSet.ScopeValues, variableSet.ScopeValues)
+}

octopusdeploy/variable_service.go

@@ -180,24 +180,27 @@ func (s variableService) DeleteSingle(ownerID string, variableID string) (Variab
 		return VariableSet{}, errInvalidvariableServiceParameter{ParameterName: "variableID"}
 	}
 
-	variables, err := s.GetAll(ownerID)
+	variableSet, err := s.GetAll(ownerID)
 	if err != nil {
 		return VariableSet{}, err
 	}
 
 	var found bool
-	for i, existingVar := range variables.Variables {
-		if existingVar.GetID() == variableID {
-			variables.Variables = append(variables.Variables[:i], variables.Variables[i+1:]...)
+	for k, v := range variableSet.Variables {
+		if v.GetID() == variableID {
+			variableSet.Variables = append(variableSet.Variables[:k], variableSet.Variables[k+1:]...)
 			found = true
 		}
 	}
 
 	if !found {
-		return VariableSet{}, ErrItemNotFound
+		return VariableSet{}, &APIError{
+			StatusCode:   404,
+			ErrorMessage: fmt.Sprintf("Variable ID, %s could not be found with owner ID, %s.", variableID, ownerID),
+		}
 	}
 
-	return s.Update(ownerID, variables)
+	return s.Update(ownerID, variableSet)
 }
 
 // Update takes an entire variable set and posts the entire set back to Octopus Deploy. There are individual
@@ -215,12 +218,15 @@ func (s variableService) Update(ownerID string, variableSet VariableSet) (Variab
 	path := trimTemplate(s.getPath())
 	path = fmt.Sprintf(path+"/variableset-%s", ownerID)
 
-	resp, err := apiUpdate(s.getClient(), variableSet, new(VariableSet), path)
-	if err != nil {
+	if _, err := apiUpdate(s.getClient(), variableSet, new(VariableSet), path); err != nil {
 		return VariableSet{}, err
 	}
 
-	return *resp.(*VariableSet), nil
+	// 2021-04-22 (John Bristowe): we need to retrieve the variable set (again)
+	// via HTTP GET (below) due to a bug for HTTP POST and HTTP PUT which will
+	// provide a null scope value set in their responses
+
+	return s.GetAll(ownerID)
 }
 
 // MatchesScope compares two different scopes to see if they match. Generally used for comparing the scope of

octopusdeploy/variable_service_test.go

@@ -46,3 +46,43 @@ func TestVariableServiceGetAllWithEmptyID(t *testing.T) {
 	require.Error(t, err)
 	require.Len(t, variableSet.Variables, 0)
 }
+
+func TestVariableServiceGetByIDWithEmptyID(t *testing.T) {
+	service := newVariableService(nil, TestURIVariables, TestURIVariableNames, TestURIVariablePreview)
+
+	variable, err := service.GetByID(emptyString, emptyString)
+	require.Error(t, err)
+	require.Nil(t, variable)
+
+	variable, err = service.GetByID(whitespaceString, emptyString)
+	require.Error(t, err)
+	require.Nil(t, variable)
+
+	variable, err = service.GetByID(emptyString, whitespaceString)
+	require.Error(t, err)
+	require.Nil(t, variable)
+
+	variable, err = service.GetByID(whitespaceString, whitespaceString)
+	require.Error(t, err)
+	require.Nil(t, variable)
+}
+
+func TestVariableServiceDeleteSingleWithEmptyID(t *testing.T) {
+	service := newVariableService(nil, TestURIVariables, TestURIVariableNames, TestURIVariablePreview)
+
+	variableSet, err := service.DeleteSingle(emptyString, emptyString)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+
+	variableSet, err = service.DeleteSingle(whitespaceString, emptyString)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+
+	variableSet, err = service.DeleteSingle(emptyString, whitespaceString)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+
+	variableSet, err = service.DeleteSingle(whitespaceString, whitespaceString)
+	require.Error(t, err)
+	require.NotNil(t, variableSet)
+}