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

go.mod,vendor,backend: update coreos/etcd dependency to v3.4.9 #25554

Closed
wants to merge 1 commit into from

Conversation

kevinburke1
Copy link
Contributor

etcd rewrote its import path from coreos/etcd to go.etcd.io/etcd/v3.
Changed the imports path in this commit.

This lets us remove the github.com/ugorji/go/codec dependency, which
was pinned to a fairly old version. The net change is a loss of 30,000
lines of code in the vendor directory. (I first noticed this problem
because the outdated go/codec dependency was causing a dependency
failure when I tried to put Terraform and another project in the same
vendor directory.)

One oddity is that the commit shows up in go.mod as 3.3.0 but is
actually after 3.4.9, there's some discussion of that (and how to
resolve it) here.
etcd-io/etcd#12068 (comment)

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 12, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #25554 (e961acb) into main (e553869) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted Files Coverage Δ
backend/remote-state/etcdv2/backend.go 0.00% <ø> (ø)
backend/remote-state/etcdv2/client.go 0.00% <ø> (ø)
backend/remote-state/etcdv3/backend.go 0.00% <ø> (ø)
backend/remote-state/etcdv3/backend_state.go 0.00% <ø> (ø)
backend/remote-state/etcdv3/client.go 0.00% <ø> (ø)
internal/logging/logging.go 30.15% <ø> (ø)
states/statefile/version4.go 58.19% <0.00%> (+0.23%) ⬆️
backend/remote/backend_common.go 50.51% <0.00%> (+0.69%) ⬆️

@kevinburke1
Copy link
Contributor Author

I updated this PR to remove the vendor directory and rebase off master. I can't look at the logs without granting CircleCI read and write access to my entire Github account, so if someone could pull the logs here I would appreciate it.

I ran the tests locally and they passed.

@pselle
Copy link
Contributor

pselle commented Jan 26, 2021

@kevinburkemeter The error here is related to generate and fmtcheck, screenshot:

Screen Shot 2021-01-26 at 11 53 25 AM

So those need to be run/ensure that fmt and generate don't result in any changed code.

@kevinburke1
Copy link
Contributor Author

Thanks, sorry, it's been about six months since I submitted the PR so I'm not surprised the tests aren't passing. Let me take a look.

@pselle
Copy link
Contributor

pselle commented Jan 27, 2021

@kevinburkemeter Thanks! I was answering your question you had here, but I want to be sure to ask -- what does upgrading the dependency solve (is it "cleaning up dependencies"?)?

If you do choose to go forward, I can request review from the etcdv3 backend maintainer (the etcdv2 backend has no maintainner currently)

@kevinburke1
Copy link
Contributor Author

I'm a little hesitant to grant CircleCI full access to every repo I have access to, so I'm unsure of the best way to view the test failure output.

@kevinburke1
Copy link
Contributor Author

what does upgrading the dependency solve (is it "cleaning up dependencies"?)?

Yes essentially, the current version of the commit that Terraform depends on imports a lot of stuff and the newer version doesn't which seems better from a depending-on-code standpoint.

@kevinburke1
Copy link
Contributor Author

I'm a little hesitant to grant CircleCI full access to every repo I have access to, so I'm unsure of the best way to view the test failure output.

Ah, I can reproduce at least one failure locally.

@pselle
Copy link
Contributor

pselle commented Jan 27, 2021

@kevinburkemeter Seems like you got it, but there's a test failure:

Failed
# github.com/hashicorp/terraform/backend/remote-state/etcdv2
../../backend/remote-state/etcdv2/backend.go:74:11: cannot use client (type *clientv3.Client) as type clientv3.Client in assignment
../../backend/remote-state/etcdv2/client.go:19:15: undefined: clientv3.NewKeysAPI
../../backend/remote-state/etcdv2/client.go:19:79: undefined: clientv3.GetOptions
../../backend/remote-state/etcdv2/client.go:21:56: undefined: clientv3.ErrorCodeKeyNotFound
../../backend/remote-state/etcdv2/client.go:39:12: undefined: clientv3.NewKeysAPI
../../backend/remote-state/etcdv2/client.go:44:12: undefined: clientv3.NewKeysAPI
panic: failed to build executable: exit status 2

goroutine 1 [running]:
github.com/hashicorp/terraform/e2e.GoBuild(0xe85c4f, 0x1e, 0xe70ba7, 0x9, 0xe8b84e, 0x1549560)
	/home/circleci/project/e2e/e2e.go:279 +0x29f
github.com/hashicorp/terraform/command/e2etest.setup(0x80)
	/home/circleci/project/command/e2etest/main_test.go:36 +0xcb
github.com/hashicorp/terraform/command/e2etest.TestMain(0xc0000d4400)
	/home/circleci/project/command/e2etest/main_test.go:15 +0x25
main.main()
	_testmain.go:137 +0x1f6
FAIL	github.com/hashicorp/terraform/command/e2etest	12.877s

I'm getting notifications on this PR so I can moderate this portion of things, I sympathize with not wanting to grant all-the-repos access :)

Additionally for backend-PRs, it's general practice to include a block of test results from running the particular backend's tests so the maintainer can see they pass (we don't have a CI setup to run each individual backend on this project)

@kevinburke1
Copy link
Contributor Author

OK, I set up a new Github account solely to grant access to CircleCI, and I'm now getting ths output, but I'm not sure it's related to my changes.

Failed
=== RUN   TestVersion
=== PAUSE TestVersion
=== CONT  TestVersion
=== CONT  TestVersion
    version_test.go:31: unexpected stderr output:
        2021-01-27 22:26:23.826603 I | [DEBUG] Adding temp file log sink: /tmp/terraform-log414029058
        2021-01-27 22:26:23.826660 I | [INFO] Terraform version: 0.15.0 dev
        2021-01-27 22:26:23.826664 I | [INFO] Go runtime version: go1.15.7
        2021-01-27 22:26:23.826676 I | [INFO] CLI args: []string{"/tmp/terraform578314163", "version"}
        2021-01-27 22:26:23.826690 I | [TRACE] Stdout is not a terminal
        2021-01-27 22:26:23.826694 I | [TRACE] Stderr is not a terminal
        2021-01-27 22:26:23.826698 I | [TRACE] Stdin is not a terminal
        2021-01-27 22:26:23.826711 I | [DEBUG] Attempting to open CLI config file: /home/circleci/.terraformrc
        2021-01-27 22:26:23.826721 I | [DEBUG] File doesn't exist, but doesn't need to. Ignoring.
        2021-01-27 22:26:23.826803 I | [DEBUG] ignoring non-existing provider search directory terraform.d/plugins
        2021-01-27 22:26:23.826813 I | [DEBUG] ignoring non-existing provider search directory /home/circleci/.terraform.d/plugins
        2021-01-27 22:26:23.826832 I | [DEBUG] ignoring non-existing provider search directory /home/circleci/.local/share/terraform/plugins
        2021-01-27 22:26:23.826842 I | [DEBUG] ignoring non-existing provider search directory /usr/local/share/terraform/plugins
        2021-01-27 22:26:23.826848 I | [DEBUG] ignoring non-existing provider search directory /usr/share/terraform/plugins
        2021-01-27 22:26:23.827070 I | [INFO] CLI command args: []string{"version"}

@kevinburke1
Copy link
Contributor Author

Okay, so, the test failures are coming because the new version of etcd also tries to call log.SetOutput in an init function, but does so after internal/logging/logging.go calls log.SetOutput (not sure exactly why the init functions run in that order) which is why the tests are failing now, because the etcd writer is winning and actually printing the output.

I think it's bad that etcd is doing that but seems like a problem that might come up again. One way to solve this would be to keep the old etcd output but maybe that's not a permanent solution, and it would be better to move the log.SetOutput call to the top-level main.go function so it can't be overridden by any other errant package.

@kevinburke1
Copy link
Contributor Author

I fixed the tests by forcing "github.com/coreos/pkg/capnslog" - which gets imported later on anyway - to import/run its init function before internal/logging/logging.go, which means that the log.SetOutput call in the latter wins.

go.mod Outdated
go.uber.org/atomic v1.3.2 // indirect
go.uber.org/multierr v1.1.0 // indirect
go.uber.org/zap v1.9.1 // indirect
go.etcd.io/etcd v0.5.0-alpha.5.0.20210123184945-d51c6c689ba3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this tag corresponds to the "release-3.4" branch of etcd, I'm not sure why Go is not able to report the tag correctly.

Copy link
Member

Choose a reason for hiding this comment

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

The issue here is that release-3.4 is not a valid version. The correct latest tag for that release appears to be v3.4.14.

Copy link
Member

Choose a reason for hiding this comment

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

OK, it looks like the etc packages are not setup correctly to be imported with a version > 1. This is the best go can generate since a version v3.4.14 cannot be imported by that name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wish the imports worked, my understanding is that the version issues will be fixed in version 3.5.

@kevinburke1
Copy link
Contributor Author

It's difficult to see without the vendor directory but the net result is a loss of about 36,000 lines of code in imported packages, if you vendor both master and this branch and run cloc on each. Master has 1,129,647 lines of third party Go code and this branch has 1,093,239, for a loss of ~36,400 lines of code.

@pselle pselle requested a review from bmcustodio February 8, 2021 20:09
@pselle
Copy link
Contributor

pselle commented Feb 8, 2021

Alright, I've requested @bmcustodio for review, who's the current etcdv3 backend maintainer, to check this one out :)

Base automatically changed from master to main February 24, 2021 18:01
@kevinburke1 kevinburke1 force-pushed the etcd branch 2 times, most recently from 21fb567 to e961acb Compare March 10, 2021 18:57
etcd rewrote its import path from coreos/etcd to go.etcd.io/etcd.
Changed the imports path in this commit, which also updates the code
version.

This lets us remove the github.com/ugorji/go/codec dependency, which
was pinned to a fairly old version. The net change is a loss of 30,000
lines of code in the vendor directory. (I first noticed this problem
because the outdated go/codec dependency was causing a dependency
failure when I tried to put Terraform and another project in the same
vendor directory.)

Note the version shows up funkily in go.mod, but I verified
visually it's the same commit as the "release-3.4" tag in
github.com/coreos/etcd. The etcd team plans to fix the release version
tagging in v3.5, which should be released soon.
@jbardin
Copy link
Member

jbardin commented Jul 20, 2021

Rebased the commit into #29200 to see if we want to merge this in.
Closing here since there has been no response from the maintainer.

@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants