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

Update github.com/ulikunitz/xz #12253

Merged
merged 3 commits into from
Sep 17, 2021
Merged

Conversation

tsaarni
Copy link
Contributor

@tsaarni tsaarni commented Aug 4, 2021

@vercel vercel bot temporarily deployed to Preview – vault August 4, 2021 17:32 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 4, 2021 17:32 Inactive
* Bump xz which is transitive dependency of github.com/mholt/archiver.
  Fixes known security vulnerability GHSA-25xm-hr59-7c27.
@vercel vercel bot temporarily deployed to Preview – vault August 18, 2021 16:14 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 18, 2021 16:14 Inactive
changelog/12253.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@pmmukh pmmukh left a comment

Choose a reason for hiding this comment

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

one minor comment, otherwise lgtm, and thanks for submitting the PR!

* Added security advisory ID to changelog.
@vercel vercel bot temporarily deployed to Preview – vault September 17, 2021 05:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 17, 2021 05:21 Inactive
@tsaarni
Copy link
Contributor Author

tsaarni commented Sep 17, 2021

@pmmukh Two tests fail connection_producer_test.go:43: Could not start docker cassandra. This seems like possible flake. Is it possible to re-trigger those two test cases?

@pmmukh
Copy link
Contributor

pmmukh commented Sep 17, 2021

ah yep, those are flaky tests, will give em a rerun!

@tsaarni
Copy link
Contributor Author

tsaarni commented Sep 17, 2021

The test seems to fail consistently now, also on my machine. Test is using bitnami/cassandra:latest which has been updated to cassandra 4.0 and startup is now much slower.

I submitted a question if this is to be expected https://github.com/bitnami/bitnami-docker-cassandra/issues/98

@tsaarni
Copy link
Contributor Author

tsaarni commented Sep 17, 2021

The test seems to fail consistently now, also on my machine. Test is using bitnami/cassandra:latest which has been updated to cassandra 4.0 and startup is now much slower.

I've now merged main to pick up #12311 which I missed in this PR. It forces fixed version bitnami/cassandra:3.11 instead of bitnami/cassandra:latest, so tests should now pass.

Another alternative that worked for me locally, even with bitnami/cassandra:4.0, would be to increase this timeout

diff --git a/helper/testhelpers/docker/testhelpers.go b/helper/testhelpers/docker/testhelpers.go
index 673a0dea7..90698e323 100644
--- a/helper/testhelpers/docker/testhelpers.go
+++ b/helper/testhelpers/docker/testhelpers.go
@@ -146,7 +146,7 @@ func (d *Runner) StartService(ctx context.Context, connect ServiceAdapter) (*Ser

        bo := backoff.NewExponentialBackOff()
        bo.MaxInterval = time.Second * 5
-       bo.MaxElapsedTime = time.Minute
+       bo.MaxElapsedTime = 2 * time.Minute

        pieces := strings.Split(hostIPs[0], ":")
        portInt, err := strconv.Atoi(pieces[1])

but I'm still unsure if this long startup time of new cassandra is to be expected or bug.

@pmmukh
Copy link
Contributor

pmmukh commented Sep 17, 2021

@tsaarni thanks for looking into that cassandra startup time issue! For the purposes of this PR, I'm going to merge this in and ignore that, I think as far as upgrading Cassandra tests to support cassandra 4, the ecosystem team will be taking a look at that later. Also, if your open issue there reaches a conclusion that some change in Vault can allow us to run tests against cassandra 4, please feel free to open a PR!

@pmmukh pmmukh merged commit e2e4b50 into hashicorp:main Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants