Skip to content

Update Go dependencies #968

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
Merged
merged 1 commit into from
Sep 21, 2020
Merged

Update Go dependencies #968

merged 1 commit into from
Sep 21, 2020

Conversation

dak1n1
Copy link
Contributor

@dak1n1 dak1n1 commented Aug 27, 2020 •
edited
Loading

Description

This PR updates Go dependencies as a prerequisite to adding the Azure provider into the Kubernetes provider (importing for acceptance tests). This also updates our other Go modules and specifically Kubernetes related libraries.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch? (If so, please include the test log in a gist)

References

#837

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@ghost ghost added the size/XXL label Aug 27, 2020
@dak1n1 dak1n1 marked this pull request as ready for review August 28, 2020 01:34
@dak1n1 dak1n1 changed the title Update Go dependencies [WIP] Update Go dependencies Aug 28, 2020
@dak1n1
Copy link
Contributor Author

dak1n1 commented Aug 28, 2020

This is still WIP. I just made it a regular PR so I can run TC tests on it.

dak1n1
dak1n1 commented Aug 28, 2020
View reviewed changes
kubernetes/provider_test.go
@@ -346,16 +363,6 @@ func clusterVersionLessThan(vs string) bool {
return cv.LessThan(v)
}

func skipCheckIf(skip func() (bool, string), check resource.TestCheckFunc) resource.TestCheckFunc {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was unused, so I removed it.

@dak1n1
Copy link
Contributor Author

dak1n1 commented Aug 28, 2020 •
edited
Loading

Acceptance tests are passing. This is ready for review.

https://gist.github.com/dak1n1/a6addda90b1f741c5b3b41a42ec3fc81

TC tests:

Here is the only new failure listed in TC. I'm not sure if it's an issue offhand. It only showed up in GKE during a cluster destroy.

 Process exited with code 1 (Step: Terraform Destroy (Command Line))  
[18:56:53]2020/08/28 18:56:53 [WARN] Log levels other than TRACE are currently unreliable, and are supported only for backward compatibility.
[18:56:53]  Use TF_LOG=TRACE to see Terraform's internal logs.
[18:56:53]  ----
[18:57:29]
[18:57:29]Error: Error deleting Cluster: Delete "https://container.googleapis.com/v1beta1/projects/hc-terraform-k8s-testing/locations/us-central1-a/clusters/tf-acc-test-dddb436eaa5d92207671?alt=json&prettyPrint=false": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
[18:57:29]
[18:57:29]
[18:57:29]Process exited with code 1

I did a couple applies and destroys using GKE from my local system, and it worked fine. It could be another TC fluke.

@dak1n1 dak1n1 changed the title [WIP] Update Go dependencies Update Go dependencies Aug 28, 2020
@dak1n1 dak1n1 requested a review from jrhouston August 28, 2020 18:12
@dak1n1 dak1n1 force-pushed the dependencies20200827 branch from 57fed0d to 62d5c01 Compare August 28, 2020 21:05
@dak1n1 dak1n1 requested review from a team and removed request for jrhouston August 31, 2020 16:05
@dak1n1
Copy link
Contributor Author

dak1n1 commented Aug 31, 2020

This is ready for review. I'm hoping to get it merged soon so I can continue working on the Azure disk PR (#837).

@aareet
Copy link
Contributor

aareet commented Aug 31, 2020

👋 I'll take a look

jrhouston
jrhouston reviewed Aug 31, 2020
View reviewed changes
Copy link
Collaborator

@jrhouston jrhouston left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @dak1n1 this is amazing work. I left a couple of comments. Main concern is with the introduction of context.Context.

@dak1n1 dak1n1 mentioned this pull request Sep 1, 2020
Closed
2 tasks
@aareet aareet mentioned this pull request Sep 4, 2020
Closed
@dak1n1 dak1n1 force-pushed the dependencies20200827 branch from 318f7eb to 254db6a Compare September 17, 2020 19:22
@dak1n1
Update Go dependencies for upcoming Azure work
566621a 
* Update Go modules & dependencies.
* Refactor Kubernetes API calls to use contexts (required by client-go version).
* Add Azure provider to acceptance tests.
* Add Azure Go modules and dependencies.
* Updates Azure test-infra to the lastest version of the Azurerm provider.
@dak1n1 dak1n1 force-pushed the dependencies20200827 branch from 3cb2801 to 566621a Compare September 17, 2020 19:43
@dak1n1
Copy link
Contributor Author

dak1n1 commented Sep 17, 2020 •
edited
Loading

Ok, I think this is finally done. All the makefile targets pass locally, except I didn't run make testacc because that takes a couple hours. Instead, I queued up these acceptance tests to cover a variety of cloud providers and kubernetes versions:

Kubernetes (AKS @ 1.17)
https://ci-oss.hashicorp.engineering/viewLog.html?buildId=147049

Kubernetes (GKE @ 1.16) (Terraform @ 0.13)
https://ci-oss.hashicorp.engineering/viewLog.html?buildId=147047

Kubernetes (GKE @ 1.15)
https://ci-oss.hashicorp.engineering/viewLog.html?buildId=147048

Kubernetes (Typhoon AWS @ 1.18)
https://ci-oss.hashicorp.engineering/viewLog.html?buildId=147065

I'll come back in a couple hours to check out the results.

@dak1n1 dak1n1 mentioned this pull request Sep 18, 2020
Merged
2 tasks
@dak1n1 dak1n1 requested a review from a team September 18, 2020 20:12
@dak1n1 dak1n1 mentioned this pull request Sep 18, 2020
Closed
2 tasks
aareet
aareet approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@aareet aareet left a comment

Choose a reason for hiding this comment

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

Thanks for working through this @dak1n1! Did a best-effort review of the changes, LGTM.

@dak1n1 dak1n1 merged commit 47f2075 into hashicorp:master Sep 21, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexsomesan alexsomesan alexsomesan left review comments

@jrhouston jrhouston jrhouston left review comments

@aareet aareet aareet approved these changes

Assignees
No one assigned
Labels
size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dak1n1 @aareet @alexsomesan @jrhouston