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.

Sign up for GitHub

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

Jump to bottom

gitlab_project: wait until real deletion #19

Merged
merged 1 commit into from
Sep 27, 2017
Merged

gitlab_project: wait until real deletion #19

merged 1 commit into from
Sep 27, 2017

Conversation

roidelapluie
Copy link
Collaborator

@roidelapluie roidelapluie commented Sep 22, 2017 •
edited
Loading

Deleting a project in Gitlab is an async method.

By blocking until the project does not exist anymore, we
do not lie to the terraform end user about the status of a
deleted project.

As a side effet it fixes the acceptances tests.

Signed-off-by: Julien Pivotto [email protected]

@roidelapluie roidelapluie changed the title Fix acceptance tests gitlab_project: wait until real deletion Sep 22, 2017
@roidelapluie
Copy link
Collaborator Author

Before

$ make testacc                                                          
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?       github.com/terraform-providers/terraform-provider-gitlab        [no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGitlabDeployKey_basic
--- PASS: TestAccGitlabDeployKey_basic (2.45s)
=== RUN   TestAccGitlabGroup_basic
--- PASS: TestAccGitlabGroup_basic (1.01s)
=== RUN   TestAccGitlabProjectHook_basic
--- FAIL: TestAccGitlabProjectHook_basic (1.27s)
        testing.go:499: Error destroying resource! WARNING: Dangling resources
                may exist. The full state and error is shown below.

                Error: Check failed: Repository still exists

                State: <no state>
=== RUN   TestAccGitlabProject_basic
--- FAIL: TestAccGitlabProject_basic (1.06s)
        testing.go:499: Error destroying resource! WARNING: Dangling resources
                may exist. The full state and error is shown below.

                Error: Check failed: Repository still exists

                State: <no state>
=== RUN   TestGitlab_validation
--- PASS: TestGitlab_validation (0.00s)
=== RUN   TestGitlab_visbilityHelpers
--- PASS: TestGitlab_visbilityHelpers (0.00s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-gitlab/gitlab 5.793s
make: *** [GNUmakefile:15: testacc] Error 1

@roidelapluie
Copy link
Collaborator Author

After

$ make testacc 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?       github.com/terraform-providers/terraform-provider-gitlab        [no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGitlabDeployKey_basic
--- PASS: TestAccGitlabDeployKey_basic (4.78s)
=== RUN   TestAccGitlabGroup_basic
--- PASS: TestAccGitlabGroup_basic (0.90s)
=== RUN   TestAccGitlabProjectHook_basic
--- PASS: TestAccGitlabProjectHook_basic (2.08s)
=== RUN   TestAccGitlabProject_basic
--- PASS: TestAccGitlabProject_basic (2.22s)
=== RUN   TestGitlab_validation
--- PASS: TestGitlab_validation (0.00s)
=== RUN   TestGitlab_visbilityHelpers
--- PASS: TestGitlab_visbilityHelpers (0.00s)
PASS
ok      github.com/terraform-providers/terraform-provider-gitlab/gitlab 9.989s

tombuildsstuff
tombuildsstuff approved these changes Sep 25, 2017
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Hey @roidelapluie

Thanks for this PR - I've taken a look and left a comment in-line but this otherwise LGTM :)

Thanks!

gitlab/resource_gitlab_project.go Outdated
return nil
}
}
time.Sleep(time.Second * 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be good to add a max attempts timeout here, if possible? i.e. try 30 times then raise an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was relying on terraform to do this, e.g. time out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isnt that a terraform core functionnality?

Copy link
Contributor

Choose a reason for hiding this comment

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

Terraform exposes this as the context but I thought it needed to be threaded through? (I may be wrong!)

In general we'd tend to achieve this using StateChangeConf to ensure it's deleted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have implemented that. Waiting for your review to merge.

@tombuildsstuff tombuildsstuff mentioned this pull request Sep 25, 2017
Merged
@roidelapluie roidelapluie force-pushed the testacc branch 2 times, most recently from 68db236 to 2986296 Compare September 26, 2017 11:48
@roidelapluie
Copy link
Collaborator Author

@tombuildsstuff is this better?

@roidelapluie
Copy link
Collaborator Author 

$ make testacc 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?       github.com/terraform-providers/terraform-provider-gitlab        [no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGitlabDeployKey_basic
--- PASS: TestAccGitlabDeployKey_basic (5.88s)
=== RUN   TestAccGitlabGroup_basic
--- PASS: TestAccGitlabGroup_basic (0.60s)
=== RUN   TestAccGitlabProjectHook_basic
--- PASS: TestAccGitlabProjectHook_basic (5.83s)
=== RUN   TestAccGitlabProject_basic
--- PASS: TestAccGitlabProject_basic (5.84s)
=== RUN   TestGitlab_validation
--- PASS: TestGitlab_validation (0.00s)
=== RUN   TestGitlab_visbilityHelpers
--- PASS: TestGitlab_visbilityHelpers (0.00s)
PASS
ok      github.com/terraform-providers/terraform-provider-gitlab/gitlab 18.151s

@roidelapluie roidelapluie mentioned this pull request Sep 26, 2017
Closed
richardc
richardc reviewed Sep 26, 2017
View reviewed changes
gitlab/resource_gitlab_project.go Outdated
return err
}

// Wait for the datacenter resource to be ready
Copy link
Collaborator

Choose a reason for hiding this comment

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

Datacenter? I suspect this is dodgy copy and paste.

@roidelapluie
gitlab_project: wait until real deletion
4381a20 
Deleting a project in Gitlab is an async method.

By blocking until the project does not exist anymore, we
do not lie to the terraform end user about the status of a
deleted project.

As a side effet it fixes the acceptances tests.

Signed-off-by: Julien Pivotto <[email protected]>
@roidelapluie roidelapluie merged commit b115bea into gitlabhq:master Sep 27, 2017
@roidelapluie
Copy link
Collaborator Author

Thanks :)

@roidelapluie roidelapluie added this to the 0.2.0 milestone Oct 1, 2017
cehoffman pushed a commit to VertivSRE/terraform-provider-gitlab that referenced this pull request Jan 23, 2018
@roidelapluie
Merge pull request gitlabhq#19 from roidelapluie/testacc
d27aa7b 
gitlab_project: wait until real deletion
@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.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

@richardc richardc richardc left review comments

Assignees
No one assigned
Labels
enhancement
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@roidelapluie @richardc @tombuildsstuff