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 sdk to 5.0.0 and add support for cidr_range #247

Conversation

fantapop
Copy link
Collaborator

@fantapop fantapop commented Nov 12, 2024

Add support for setting the cidr_range value for GCP clusters.

This is ready for review. Once the SDK is tagged, I'll need to update a few files but nothing substantial will change.

As I was working through the tests, I found some other issues related to previous PRs. I included them as separate commits in this one but I can pull them out to separate PRs if desired.

Commit checklist

  • Changelog
  • Doc gen (make generate)
  • Integration test(s)
  • Acceptance test(s)
  • Example(s)

@fantapop fantapop force-pushed the fantapop/CC-30497-terraform-support-cidr-range-attribute-on-cluster-creation branch 2 times, most recently from 4893f39 to 5656c89 Compare November 14, 2024 00:04
@fantapop fantapop marked this pull request as ready for review November 14, 2024 00:07
@fantapop fantapop requested a review from rgcase November 14, 2024 00:07
@fantapop fantapop changed the title Update sdk to 4.2.0 and add support for cidr_range Update sdk to 5.0.0 and add support for cidr_range Nov 14, 2024
Copy link

@kathancox kathancox left a comment

Choose a reason for hiding this comment

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

.md files LGTM

Copy link
Contributor

@rgcase rgcase left a comment

Choose a reason for hiding this comment

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

LGTM

Previously a cluster test was creating a backup configuration that
didn't specify the RetentionDays directly.  This meant that it would
rely on the server specified default.  In the test case, it was then
compared to a local object which had a value different from the default
which was causing the test to fail.

Rather than change the local value to match the server side default, we
instead pass the local value into the test so that it is created with
the non-default value.
Previously, the testDedicatedClusterResource test was requesting 2VCPU
clusters which was failing during an actual acceptance test.  This
updates it to use 4VCPU.  We additionally had to increase the scale step
to 8VCPU to provide a real value that would necessitate an update.

additionaly, we update the default from 2 to 4 in the advanced example.
@fantapop fantapop force-pushed the fantapop/CC-30497-terraform-support-cidr-range-attribute-on-cluster-creation branch from 5656c89 to 4caea03 Compare November 14, 2024 18:24
We update the cockcroach-cloud-sdk-go to get changes for supporting
cidr_range in clusters as well as JWT authentication in the client.
@fantapop fantapop force-pushed the fantapop/CC-30497-terraform-support-cidr-range-attribute-on-cluster-creation branch from 4caea03 to 5263ba3 Compare November 14, 2024 18:26
@fantapop
Copy link
Collaborator Author

I ran the acceptance tests on this branch here. It failed due to a flake on the Creating a SQL user. (datadog)

I'm going to go ahead merge it down.

@fantapop fantapop merged commit ab4f78b into main Nov 14, 2024
4 of 5 checks passed
@fantapop fantapop deleted the fantapop/CC-30497-terraform-support-cidr-range-attribute-on-cluster-creation branch November 14, 2024 23:15
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.

3 participants