terraform-provider-teamcity icon indicating copy to clipboard operation
terraform-provider-teamcity copied to clipboard

Added support for _Root project parameters and updated README.md

Open sundarok opened this issue 4 years ago • 5 comments

  • Added new resource "resource_root_project" to support the parameters in _Root project level.
  • Since the _Root project is already exists in TeamCity the current resource will not be used. Need a special resource that will add/update/delete only the _Root level configuration parameters. The same should be handled while destroying the resource also.
  • Added tests for the Root project resource

sundarok avatar Jun 09 '20 17:06 sundarok

Hi again 👋 @sundarok and @justinto-guidewire, after giving this a closer look, I've tried an approach that I'd like to run by you.

It involves adding a new field to the resource_project schema:

"root": {
	Type: schema.TypeBool,
	Optional: true,
	Default: false,
}

I've began by writing an acceptance test and tried to make it work. Note that this test in particular doesn't have some checks such as CheckDestroy: testAccCheckTeamcityProjectDestroy

func TestAccTeamcityProject_Root(t *testing.T) {
	resName := "teamcity_project.root"
	var p api.Project

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		Steps: []resource.TestStep{
			resource.TestStep{
				Config: testAccTeamcityProjectRoot,
				Check: resource.ComposeTestCheckFunc(
					testAccCheckTeamcityProjectExists(resName, &p),
					resource.TestCheckResourceAttr(resName, "name", "<Root project>"),
					resource.TestCheckResourceAttr(resName, "description", "Contains all other projects"),
					resource.TestCheckResourceAttr(resName, "root", "true"),
					resource.TestCheckResourceAttr(resName, "config_params.param1", "config_value1"),
					resource.TestCheckResourceAttr(resName, "config_params.param2", "config_value2"),
					resource.TestCheckResourceAttr(resName, "env_params.param3", "env_value1"),
					resource.TestCheckResourceAttr(resName, "env_params.param4", "env_value2"),
					resource.TestCheckResourceAttr(resName, "sys_params.param5", "sys_value1"),
					resource.TestCheckResourceAttr(resName, "sys_params.param6", "sys_value2"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.Configuration, "param1", "config_value1"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.Configuration, "param2", "config_value2"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.EnvironmentVariable, "param3", "env_value1"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.EnvironmentVariable, "param4", "env_value2"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.System, "param5", "sys_value1"),
					testAccCheckProjectParameter(&p, api.ParameterTypes.System, "param6", "sys_value2"),
				),
			},
		},
	})
}
const testAccTeamcityProjectRoot = `
resource "teamcity_project" "root" {
  	root = true

	config_params = {
		param1 = "config_value1"
		param2 = "config_value2"
	}

	env_params = {
		param3 = "env_value1"
		param4 = "env_value2"
	}

	sys_params = {
		param5 = "sys_value1"
		param6 = "sys_value2"
	}
}
`

This would indicate to the config that this is a root project. However, we have a conflict problem with name and description, fields that cannot be ever set for the Root project. Terraform doesn't have support for ConflictsWith to evaluate resource field values. In order to circumvent that, I've looked for examples in other provider codebases and I believe CustomizeDiff can help in this case.

const (
	ResourceProjectRootId   = "_Root"
	ResourceProjectRootName = "<Root project>"
	ResourceProjectRootDescription = "Contains all other projects"
)
...
CustomizeDiff: func(diff *schema.ResourceDiff, i interface{}) error {
	root := diff.Get("root").(bool)
	if root {
		if v, ok := diff.GetOk("name"); ok && v.(string) != ResourceProjectRootName {
			return errors.New("'name' cannot be defined for the root project")
		}
		if v, ok := diff.GetOk("description"); ok && v.(string) != ResourceProjectRootDescription {
			return errors.New("'description' cannot be defined for the root project")
		}
	}
	// Validate required name if root = false
	if _, ok := diff.GetOk("name"); !ok && !root {
		return errors.New("'name' is required for non-root project")
	}

	return nil
},

Some other adjustments had to be made in order for all the tests to pass, like skipping destroy (but removing from the state with d.SetId("")), and fixing some panics regarding ParentProject.

I'd say this option could be highly beneficial avoiding the creation of another resource, and also doesn't add that much complexity. It struck me as intuitive and easy for the end user, too.

What do you say?

cvbarros avatar Jun 12 '20 11:06 cvbarros

Hi @cvbarros, we agree with your approach as well, could you provide the changes you mentioned in the above comments and we can add any missing tests or anything else, and then update this PR for you to review. Thanks!

justinto-guidewire avatar Jun 12 '20 14:06 justinto-guidewire

👋 @justinto-guidewire I've pushed a branch with the idea: f6f4db8a6b0c001290ec9e4fce1b08f719bee8d1 However, I'd be way more happy if the contributions came from the original authors 👍

cvbarros avatar Jun 12 '20 17:06 cvbarros

Hello @sundarok @justinto-guidewire , there hasn't been much activity here since we last spoke. Is this something you'd still be willing to contribute/move forward?

Thanks!

cvbarros avatar Oct 13 '20 08:10 cvbarros

Hi @cvbarros , unfortunately, we have other priorities at the moment, and won't be contributing to add root project parameters anymore. Please feel free to close this PR, thanks!

justinto-guidewire avatar Oct 13 '20 15:10 justinto-guidewire