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

feat: Connection resource #3162

Merged
merged 46 commits into from
Nov 7, 2024
Merged

feat: Connection resource #3162

merged 46 commits into from
Nov 7, 2024

Conversation

sfc-gh-fbudzynski
Copy link
Collaborator

@sfc-gh-fbudzynski sfc-gh-fbudzynski commented Oct 29, 2024

Changes

  • connection resource
  • connection documentation
  • added connection to migration guide

Test Plan

  • acceptance tests

TODO

References

MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
docs/resources/connection.md Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/connection_acceptance_test.go Outdated Show resolved Hide resolved
pkg/resources/connection_acceptance_test.go Outdated Show resolved Hide resolved
Copy link

Integration tests success for 9becdb93a39fbcdf01c0293eef45adbe3992ef85

Copy link

Integration tests failure for 9ed9076c0d3be5ec5f3154b9bed97a73b785c905

MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
docs/resources/secondary_connection.md Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/secondary_connection.go Outdated Show resolved Hide resolved
pkg/sdk/connections_gen_test.go Outdated Show resolved Hide resolved
pkg/sdk/connections_impl_gen.go Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/secondary_connection.go Outdated Show resolved Hide resolved
pkg/sdk/connections_impl_gen.go Outdated Show resolved Hide resolved
pkg/resources/secondary_connection.go Show resolved Hide resolved
pkg/provider/resources/resources.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 4, 2024

Integration tests success for f86d26e1fed91cf49852de03060872a21a6c4b04

Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki left a comment

Choose a reason for hiding this comment

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

For me, all the nits and comments (from my review) can be handled in the next separate PR


Added a new resources for managing connections. We decided to split connection into two separate resources based on whether the connection is primary or a replica (secondary). i.e.:

- `snowflake_connection` is used as primary connection, with ability to enable failover to other accounts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: snowflake_primary_connection

- `snowflake_connection` is used as primary connection, with ability to enable failover to other accounts.
- `snowflake_secondary_connection` is used as replica (secondary) connection.

In order to promote secondary_connection to primary, resources need to be migrated (check [resource migration](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md)) or re-created and imported using the following SQL statements on Snowflake Worksheet:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this is fine to have this info in the migration guide, but eventually, it would be great to also include this in the resource docs (it's important information for later)

// TODO: [SNOW-1763442] unable to test now, as there is no test accounts with different regions
// RecreateWhenSecondaryConnectionChangedExternally detects if the secondary connection was promoted externally to serve as primary.
// If so, it sets the `is_primary` field to `false` which is our desired value for secondary_connection
func RecreateWhenSecondaryConnectionPromotedExternally() schema.CustomizeDiffFunc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we have to handle this the other way around too for primary connection demotion

@@ -2,11 +2,10 @@ package resources_test

import (
"context"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: imports ordering

func RecreateWhenSecondaryConnectionPromotedExternally() schema.CustomizeDiffFunc {
return func(_ context.Context, diff *schema.ResourceDiff, _ any) error {
if _, newValue := diff.GetChange("is_primary"); newValue.(bool) {
return diff.SetNew("is_primary", false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmm, this logic is not aligned with others (check RecreateWhenUserTypeChangedExternally, RecreateWhenSecretTypeChangedExternally, or RecreateWhenResourceTypeChangedExternally) - there is ForceNew missing - it's a problem but it can be handled in the next PR too

func TestAcc_PrimaryConnection_Basic(t *testing.T) {
// TODO: [SNOW-1002023]: Unskip; Business Critical Snowflake Edition needed
_ = testenvs.GetOrSkipTest(t, testenvs.TestFailoverGroups)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add

_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)
acc.TestAccPreCheck(t)

to all the tests that use TestClient in their set-up

pkg/sdk/parsers.go Show resolved Hide resolved
Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak left a comment

Choose a reason for hiding this comment

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

Approved. There are still open threads and new comments that are not crucial and can be applied in the next pr.

- `snowflake_connection` is used as primary connection, with ability to enable failover to other accounts.
- `snowflake_secondary_connection` is used as replica (secondary) connection.

In order to promote secondary_connection to primary, resources need to be migrated (check [resource migration](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md)) or re-created and imported using the following SQL statements on Snowflake Worksheet:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good, but slightly rephrase that In order to promote secondary_connection to primary, resources need to be removed from the state, re-created manually using the following SQL statements ... then imported again, but now as snowflake_connection (check resource migration).

pkg/resources/custom_diffs_test.go Show resolved Hide resolved
pkg/schemas/connection_gen.go Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
pkg/resources/connection.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 5, 2024

Integration tests failure for e779da4cdcae7f1e10dbc1644b0e1111008bbc21

Copy link

github-actions bot commented Nov 5, 2024

Integration tests failure for 1263a916756ab874a764f57e965e2d69288b3e31

@sfc-gh-fbudzynski sfc-gh-fbudzynski merged commit 5aef117 into main Nov 7, 2024
8 of 9 checks passed
@sfc-gh-fbudzynski sfc-gh-fbudzynski deleted the connection-resource branch November 7, 2024 08:40
sfc-gh-jcieslak added a commit that referenced this pull request Nov 8, 2024
<!-- Feel free to delete comments as you fill this in -->

<!-- summary of changes -->
## Changes
* fix secrets datasource acceptance tests
* added tests for `ParseCommaSeparatedAccountIdentifierArray`
* added customDiff and tests for re-creating connection resources
* adjusted migration-guide and documentation for connection resources
* added 'TestAccPreCheck' to acceptance tests

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] unit tests
* [x] integration tests
* [x] acceptance tests
* [x] acceptance tests for secrets-datasource

## References
<!-- issues documentation links, etc  -->
*
#3162

---------

Co-authored-by: Jan Cieślak <[email protected]>
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
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.

4 participants