Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instance clusters to Terraform provider #367

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

sfang97
Copy link
Contributor

@sfang97 sfang97 commented Jul 21, 2020

Add support for instance level cluster management via the Gitlab Terraform provider. Since instance cluster API has been added to Gitlab, instance cluster management should also be added to Terraform provider. The API implementation is in the upstream package

@sfang97 sfang97 marked this pull request as draft July 21, 2020 17:14
@ghost ghost added the documentation label Jul 21, 2020
@sfang97
Copy link
Contributor Author

sfang97 commented Aug 1, 2020

Currently the CI checker is failing because "Unchanged files with check annotations" in vendor/github.com/xanzy/go-gitlab/labels.go. When I run go mod vendor a few files get modified including labels.go, and then when I run make build I get the Error: gitlab/resource_gitlab_label.go:72:81: cannot use promoted field ListOptions.Page in struct literal of type gitlab.ListLabelsOptions. After I git restore labels.go, make build passes. I'd appreciate any insight on how to fix this!

@sfang97 sfang97 marked this pull request as ready for review August 1, 2020 22:28
@sfang97 sfang97 changed the title WIP: Add instance clusters to Terraform provider Add instance clusters to Terraform provider Aug 1, 2020
@stanhu
Copy link
Member

stanhu commented Aug 2, 2020

@sfang97 Try this:

diff --git a/gitlab/resource_gitlab_label.go b/gitlab/resource_gitlab_label.go
index de1a3fe..f2fe533 100644
--- a/gitlab/resource_gitlab_label.go
+++ b/gitlab/resource_gitlab_label.go
@@ -69,7 +69,7 @@ func resourceGitlabLabelRead(d *schema.ResourceData, meta interface{}) error {
 	page := 1
 	labelsLen := 0
 	for page == 1 || labelsLen != 0 {
-		labels, _, err := client.Labels.ListLabels(project, &gitlab.ListLabelsOptions{Page: page})
+		labels, _, err := client.Labels.ListLabels(project, &gitlab.ListLabelsOptions{ListOptions: gitlab.ListOptions{Page: page}})
 		if err != nil {
 			return err
 		}

See golang/go#9859 for more details.

@stanhu
Copy link
Member

stanhu commented Aug 2, 2020

I'm also wondering if the build weirdness is caused by this error that I got:

$ make
==> Checking that code complies with gofmt requirements...
go install
go: inconsistent vendoring in /Users/stanhu/github/terraform-provider-gitlab:
	github.com/xanzy/go-gitlab@v0.32.1: is replaced in go.mod, but not marked as replaced in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory

@sfang97
Copy link
Contributor Author

sfang97 commented Aug 2, 2020

@stanhu Thanks Stan, I tried replacing the line but no dice. The issue seems to be coming from the changes to label.go in the go-gitlab upstream package that aren't in the vendor file-- the last commit to the vendor version was 3 months ago, and the last commit to the go-gitlab package was 21 days ago, so I'm going to guess that something about the most recent change is making the compiler unhappy. I'll dig around in the diffs more to see if I can figure out what's going on.

As for the weird go mod vendor error, the command is supposed to make a copy of the packages needed for the build/tests, so I can tell that it's trying to replace github.com/xanzy/go-gitlab v0.32.1 with github.com/sfang97/go-gitlab v0.33.1-0.20200731201522-09106effb0dc (< my unmerged change) which is correct. I'm not sure how to run that command in the CI check though?

@sfang97 sfang97 force-pushed the instance-cluster-gitlab-terraform branch from 56204fe to b71db56 Compare August 8, 2020 00:42
@sfang97
Copy link
Contributor Author

sfang97 commented Aug 18, 2020

@roidelapluie Hi there Julien! 👋 Could you review this PR?

Copy link
Collaborator

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some comments

website/gitlab.erb Outdated Show resolved Hide resolved
gitlab/resource_gitlab_instance_cluster_test.go Outdated Show resolved Hide resolved
gitlab/resource_gitlab_instance_cluster_test.go Outdated Show resolved Hide resolved
@sfang97
Copy link
Contributor Author

sfang97 commented Aug 21, 2020

@roidelapluie Thanks for the review Julien, I've applied the changes-- take another look? 🏓

@nicholasklick
Copy link
Collaborator

@armsnyder mind taking a pass at this?

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left some clarifying questions and minor suggestions for ya.

gitlab/resource_gitlab_instance_cluster.go Show resolved Hide resolved
gitlab/resource_gitlab_instance_cluster.go Outdated Show resolved Hide resolved
gitlab/resource_gitlab_instance_cluster_test.go Outdated Show resolved Hide resolved
gitlab/resource_gitlab_instance_cluster_test.go Outdated Show resolved Hide resolved
@armsnyder
Copy link
Collaborator

Looks like this MR needs a rebase as well.

@sfang97 sfang97 force-pushed the instance-cluster-gitlab-terraform branch 2 times, most recently from bfdcb3f to 1026fd6 Compare September 8, 2020 19:42
@sfang97
Copy link
Contributor Author

sfang97 commented Sep 11, 2020

@armsnyder Thanks for the reviews Adam, I've applied the non-breaking changes and temporarily reverted the ones that are waiting for the go-gitlab changes to go in. Once those go in I can open a new PR in the terraform repo to reflect those changes. Take another look? :) 🏓

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nicholasklick
Copy link
Collaborator

So can we merge this?

@sfang97
Copy link
Contributor Author

sfang97 commented Sep 14, 2020

@nicholasklick I think this can be merged, @armsnyder @roidelapluie what do y'all think?

@armsnyder
Copy link
Collaborator

@sfang97 Go ahead, unless you're waiting for @roidelapluie's feedback.

@armsnyder
Copy link
Collaborator

@sfang97 plz squash-merge lol 80 commits

author Ringo De Smet <ringo@de-smet.name> 1592760067 +0200
committer sfang97 <sfang@gitlab.com> 1600137187 -0500

parent 1cbab81
author Ringo De Smet <ringo@de-smet.name> 1592760067 +0200
committer sfang97 <sfang@gitlab.com> 1600137111 -0500

parent 1cbab81
author Ringo De Smet <ringo@de-smet.name> 1592760067 +0200
committer sfang97 <sfang@gitlab.com> 1600136946 -0500

Fix required Go version

Add instance clusters to Terraform provider

Add support for instance level cluster management via the Gitlab
Terraform provider.

Add instance cluster to provider

Add instance cluster go file

Add instance clusters to website

Only test read instance cluster for now

Add functionality to CRUD

Tests mostly passing now, except a weird invalid URL escape thing, will
investigate this

Remove instance_clusters.go from this PR

Contributed the instance API file to the upstream repo
github.com/xanzy/go-gitlab, so removing from this PR

Readding instance_clusters for failed pipeline

Replace required libary in go mod

Replace vendor with upstream in go mod

Remove vendor files

Remove changes to gitlab go

Remove instance clusters from README

Add management project param back in

Rerun CI build

Removing line for CI build

Rerun CI pipeline

Undo change to xanzy README

Undo another change to README

Change the commit to replace go-gitlab requirement

Add go mod vendor to travis

Move go mod vendors command to lint stage

Change function names based on vendor change

Remove replace package

feat: Add support to create projects from templates

Undo cherry pick

This reverts commit 5191839.

Update go mod to latest go-gitlab

Remove changes to travis

Remove newline for new pipeline

Update docs, no instance ID

Add backlashes so no italics

Remove instance from example usage

Remove white space, change instance link

Address PR comments

Add forcenew back to auth type

run make fmt

Prepare changelog for release

v2.11.0

Cleanup after v2.11.0 release

Fix Jira Service error handling (gitlabhq#363)

Fix nil pointer dereferences during error handling (gitlabhq#364)

* Fix error handling when deleting deploy key
There was the possibility of a panic if response was nil, and the GitLab client already treats the 204 code as a success, so the extra check was not needed.

* Fix nil pointer dereference issues in error messages

correct management_project_id argument in documentation (gitlabhq#368)

Fix incorrect description of valid access levels (gitlabhq#342)

This minor change corrects a mistake showing `master` as a valid level
of `group_access_level`.

Import group labels (gitlabhq#339)

* add support for importing gitlab group labels

* fix resource name in the acceptance test

* convert group ID to string

Co-authored-by: David Townshend <david.townshend@allangray.co.za>

Add importer to resource_gitlab_project_push_rules (gitlabhq#360)

* Add importer to resource_gitlab_project_push_rules

* Fix apparent copy-paste problem and inconsistency in push rules debug logs

Bump go-gitlab to v.0.34.1 (gitlabhq#378)

* Fix variable passed to ListLabelsOptions

Needed to fix nested struct format

* Don't need go mod changes

* Add back go mod

* Bump to 0.34.1

* Add 0.34.1 to modules.txt

* Revert "Add 0.34.1 to modules.txt"

This reverts commit 2ac4e3e.

* Add vendor files for bump

Co-authored-by: sfang97 <sfang@gitlab.com>

Renamed the Github organization after the migration to https://github.com/gitlabhq

Change isRunningInEE test

Fix import packages

Call configure in provider test

Only initialize provider in acceptance tests

Remove undefined resource reference

Add undefined testenvvar

Had to run make fmt

Remove unused variable

Reference testenvvar resource correctly

Add group_ldap_link to index

Signed-off-by: Sune Keller <absukl@almbrand.dk>

Adding project miorror and tests

responding to PR comments, removing import state verify for acceptance test

Setting all properties in SetToState

removed commented out ignore

adding docs page

adding to index

Store more attributes into the state

Fix missing test checks in the resource user

Allow to change user email address without creating a new resource

Fix the external flag being mapped to admin instead

Switching the external flag makes tests fail

Add new data source gitlab_group_membership

Allow to filter members by their access_level

Add new doc page to gitlab.erb

Update website/gitlab.erb

Co-authored-by: Adam Snyder <armsnyder@gmail.com>

Added Quotes in the example for group cluster

Prepare changelog for release

Cleanup after v2.11.0 release

Fix Jira Service error handling (gitlabhq#363)

Fix nil pointer dereferences during error handling (gitlabhq#364)

* Fix error handling when deleting deploy key
There was the possibility of a panic if response was nil, and the GitLab client already treats the 204 code as a success, so the extra check was not needed.

* Fix nil pointer dereference issues in error messages

correct management_project_id argument in documentation (gitlabhq#368)

Fix incorrect description of valid access levels (gitlabhq#342)

This minor change corrects a mistake showing `master` as a valid level
of `group_access_level`.

Import group labels (gitlabhq#339)

* add support for importing gitlab group labels

* fix resource name in the acceptance test

* convert group ID to string

Co-authored-by: David Townshend <david.townshend@allangray.co.za>

Add importer to resource_gitlab_project_push_rules (gitlabhq#360)

* Add importer to resource_gitlab_project_push_rules

* Fix apparent copy-paste problem and inconsistency in push rules debug logs

Bump go-gitlab to v.0.34.1 (gitlabhq#378)

* Fix variable passed to ListLabelsOptions

Needed to fix nested struct format

* Don't need go mod changes

* Add back go mod

* Bump to 0.34.1

* Add 0.34.1 to modules.txt

* Revert "Add 0.34.1 to modules.txt"

This reverts commit 2ac4e3e.

* Add vendor files for bump

Co-authored-by: sfang97 <sfang@gitlab.com>

Change isRunningInEE test

Fix import packages

Call configure in provider test

Only initialize provider in acceptance tests

Remove undefined resource reference

Add undefined testenvvar

Had to run make fmt

Remove unused variable

Reference testenvvar resource correctly

Add group_ldap_link to index

Signed-off-by: Sune Keller <absukl@almbrand.dk>

Adding project miorror and tests

responding to PR comments, removing import state verify for acceptance test

Setting all properties in SetToState

removed commented out ignore

adding docs page

adding to index

Store more attributes into the state

Fix missing test checks in the resource user

Allow to change user email address without creating a new resource

Fix the external flag being mapped to admin instead

Switching the external flag makes tests fail

Allow to filter members by their access_level

Update website/gitlab.erb

Co-authored-by: Adam Snyder <armsnyder@gmail.com>

Added Quotes in the example for group cluster

Remove redundant test func

Add managed test

Address PR comments

Revert "Address PR comments"

This reverts commit a963eaf.

Remove managed and enabled for now
@sfang97 sfang97 force-pushed the instance-cluster-gitlab-terraform branch from ff23675 to a9bf04b Compare September 15, 2020 02:33
@armsnyder
Copy link
Collaborator

armsnyder commented Sep 15, 2020

@sfang97 That's fine how you did it, but for future reference, I meant that when you merge this PR from the GitHub UI there is an option to squash. That way you don't risk making any mistakes after the PR's already approved, too.

@sfang97
Copy link
Contributor Author

sfang97 commented Sep 15, 2020

@armsnyder Oops, understood! Thanks for the tip :)

@sfang97 sfang97 merged commit 4bd90e0 into gitlabhq:master Sep 15, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

6 participants