feat(github_team_repository): allow for custom repository roles

[joshua-hancox]

This PR is based on the PR here:

https://github.com/integrations/terraform-provider-github/pull/1038/files

I am waiting for this functionality and so would like to get it merge ASAP, if possible without waiting for a major version.

The previous PR is still valid since changing the default is a breaking change that can be merged before the next major release.

Previous PR details:

Allow custom repository roles to be passed to the github_team_repository resource by removing the string validation on the permission argument.

Update the default permission to push, which is the default for the newer API resource; previously the API pulled the default from the (deprecated) permission parameter of the team resource. This may cause an unintended change for some provider users, so I'll leave this up for the maintainers to decide whether to include or not.

I also opted to leave the tests unchanged as "pull", "triage", "push", "maintain", "admin" are still valid test cases.

Closes: #988

[joshua-hancox]

@kfcampbell do you mind taking a look at this one. Apologies for the trying to hurry things along but this is a functionality I would like to use.

[reedloden]

Is there a way to pull all possible roles from GitHub and then validate against that?

[joshua-hancox]

I am sure we could use this API for that.

Do you know of an example where something similar is done before? Or could you suggest where I should create the function. First time contributor :)

[joshua-hancox]

Seems like we do some other validation in utils.go (e.g. validateValueFunc and validateTeamIDFunc) but I don't see anyone calling back to github api here.

[joshua-hancox]

I think we probably need to wait for this: google/go-github#2336? But I am also a little out of my depth here.

[btsteve]

The go-github functionality for merged a little while ago.

[kfcampbell]

It's merged but unfortunately has not been released yet; the last release of the library was March 1st. We'll have to wait for their next official release to use the functionality in go-github#2336.

@joshuahancox is correct and thus far we've chosen to hard-code those validation functions rather than use extra API calls. I'm a little hesitant to change that approach here, especially as we tend to see API overuse on more intensive Terraform runs.

[reedloden]

#1138 will bump go-github to include the necessary functionality.

[kfcampbell]

I've just updated and merged #1138!

[stefansedich]

Just saw this replacement PR for this functionality, big +10000 from me to get this one in, it is something that is currently blocking us assigning users a custom role with dependabot permissions. If there is anything I can do to help let me know!

[joshua-hancox]

Is there anything else I can do to help get this in? :)

[kfcampbell]

@joshuahancox thank you for the offer! The best way to speed this up would be to use the GitHub API to validate the roles created as suggested here. I've merged the PR that updates google/go-github to the required version.

[joshua-hancox]

@joshuahancox thank you for the offer! The best way to speed this up would be to use the GitHub API to validate the roles created as suggested here. I've merged the PR that updates google/go-github to the required version.

Thanks @kfcampbell, @reedloden,

My idea would have been to include a validateTeamRepositoryPermissionFunc() in util.go and validate from the API inside that function, and then use that as the ValidateFunc in the resourceGithubTeamRepository() function inside resource_github_team_repository.go.

However I can't pass the meta interface{} required to interact with the API due to the type SchemaValidateFunc requiring only a single interface (field value) and a single string (field name).

I can't see any other example of interacting with the api for validation in the codebase, so unsure where I should implement this. Could you give a little guidance please?

Apologies for the amount of help needed.

[joshua-hancox]

Also, just realised that there is a problem with the function from go-github - so have raised a pr there too.

https://github.com/google/go-github/pull/2370/files

[joshua-hancox]

Added an example of what I was talking about here - but obviously this isn't working code right now. Could you let me know if this is something similar to what you were thinking or if I am way off? :)

@reedloden
@kfcampbell

[reedloden]

@joshuahancox Don't forget you need to check against the superset of default roles + any custom roles. As currently written, the proposed change only checks against custom roles, no?

[joshua-hancox]

@reedloden yes of course, updated thankyou!

Any idea about my other questions regarding the type SchemaValidateFunc? What should I do there?

[joshua-hancox]

Thanks @reedloden I committed your further suggestion. Any idea on how to handle my other question?

[reedloden]

Thanks @reedloden I committed your further suggestion. Any idea on how to handle my other question?

Sorry, I'm just an outside lurker, so not sure of the best approach. One of the core developers would likely have a better idea on how to handle it.

[joshua-hancox]

Thanks @reedloden I committed your further suggestion. Any idea on how to handle my other question?

Sorry, I'm just an outside lurker, so not sure of the best approach. One of the core developers would likely have a better idea on how to handle it.

No worries! Thankyou for the help so far anyway. @kfcampbell could you advise me a little further? Or do you know someone that I could @ for some guidance?

#1113 (comment)

[joshua-hancox]

@jcudit @radeksimko @anGie44 @kfcampbell

Does anyone have any idea what I should do here? We would like to validate the permission that we are setting on the team/repository resource here against custom roles returned from the github list custom roles API + and the known existing roles, however the type SchemaValidateFunc does not allow for passing the meta interface{} required to interact with the API.

I can't see a way to do the validation, should we validate in a different way, or remove the validation altogether (as in the original PR)?

[anGie44]

Hi @joshuahancox , afik SchemaValidateFunc doesn't give you access to the meta unfortunately but a CustomizeDiff does, with the limitation that it returns an apply-time error I believe instead of at plan-time like when using the validatefunc so that may affect the UX as users usually like to know input errors before apply. You could potentially do something like the following where you transform the current method already in util.go:

func resourceGithubTeamRepository() *schema.Resource {
	return &schema.Resource{
		...

		CustomizeDiff: customdiff.Sequence(
			resourceTeamRepositoryPermissionCustomizeDiff,
		),
	}
}

func RepoRoles_Values() []string {
	return []string{
		"pull",
		"triage",
		"push",
		"maintain",
		"admin",
	}
}

func resourceTeamRepositoryPermissionCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error {
	if roleName := diff.Get("permission").(string); roleName != "" {
		roles := RepoRoles_Values()

		client := meta.(*Owner).v3client
		orgName := meta.(*Owner).name
		ctx := context.Background()

		customRoles, _, err := client.Organizations.ListCustomRepoRoles(ctx, orgName)

		if err != nil {
			ghErr, ok := err.(*github.ErrorResponse)
			if !ok {
				return fmt.Errorf("Error in response from github while checking for existing custom roles. Error: %s", err)
			}
			
			if ghErr.Response.StatusCode != http.StatusNotFound {
				return fmt.Errorf("Error in response from github while checking for existing custom roles. Error: %s", ghErr.Error())
			}
		}

		if customRoles != nil {
			for _, role := range customRoles.CustomRepoRoles {
				roles = append(roles, *role.Name)
			}
		}
		
		valid := false
		for _, role := range roles {
			if role == roleName {
				valid = true
				break
			}
		}

		if !valid {
			return fmt.Errorf("A role with the name %s does not exist in this GitHub organisation", roleName)
		}
	}

	return nil
}

The error handling there is just as an example as I'm not too sure if you'd want to always error out when no custom roles are available. Hope this helps though!

[kfcampbell]

I apologize I haven't been around much. @grant-adarga has updated his PR to remove the validation entirely, which I'll handle merging/releasing as-is for now.

I agree it's obnoxious we can't get client/owner information in SchemaValidationFuncs. It would be great to have access to the meta there. In the meantime, probably the best course of action would be bumping to the next Terraform SDK version and use a SchemaValidateDiagFunc to bubble up a warning to the user if their role isn't a default one and hope they have a custom one configured.