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.

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

feat: Rework provider configuration fields #3152

Merged
merged 6 commits into from
Oct 30, 2024
Merged

feat: Rework provider configuration fields #3152

merged 6 commits into from
Oct 30, 2024

Conversation

sfc-gh-jmichalak
Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak commented Oct 23, 2024 •
edited
Loading

  • extract env names
  • add an entry to migration guide
  • add tests for invalid configurations
  • improve validations

Test Plan

  • acceptance tests
  • …

References

Open topics

  • Do we want to move doc helpers to a common package? We could reuse some functions with the resource package.

TODO

  • add new fields from the doc
  • more advanced testing will be done in tickets for auth flows and for config hierarchy
  • use helpers to get values from the schema

@github-actions GitHub Actions
Copy link

Integration tests cancelled for 92ac5c04f1c75e2b274a1b7f55823e5f5f5dc90a

@github-actions GitHub Actions
Copy link

Integration tests failure for 2e641f60b6ae942480f64a9bf14816e25bf41563

@sfc-gh-jmichalak sfc-gh-jmichalak marked this pull request as ready for review October 24, 2024 06:34
@sfc-gh-jmichalak sfc-gh-jmichalak requested review from sfc-gh-asawicki and sfc-gh-jcieslak and removed request for sfc-gh-asawicki October 24, 2024 06:35
sfc-gh-asawicki
sfc-gh-asawicki requested changes Oct 24, 2024
View reviewed changes
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/internal/snowflakeenvs/snowflake_environment_variables.go Show resolved Hide resolved
pkg/provider/provider_helpers.go Show resolved Hide resolved
pkg/provider/provider_helpers.go Outdated Show resolved Hide resolved
pkg/provider/provider_helpers.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated
@@ -734,25 +753,34 @@ func ConfigureProvider(s *schema.ResourceData) (interface{}, error) {
config.DisableTelemetry = v.(bool)
}

if v, ok := s.GetOk("client_request_mfa_token"); ok && v.(bool) {
config.ClientRequestMfaToken = gosnowflake.ConfigBoolTrue
if v := s.Get("client_request_mfa_token").(string); v != resources.BooleanDefault {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: for the next PR (or even the next one); we already have some helpers in the resources that allow as making such blocks of code much shorter and more readable, it does not differ on the conceptual level from what happens in the resources' create methods. It would be great to extract such methods and reuse them. This logic will be much more compact and much less repetitive then

pkg/provider/provider_acceptance_test.go Show resolved Hide resolved
pkg/provider/provider_acceptance_test.go Show resolved Hide resolved
sfc-gh-jcieslak
sfc-gh-jcieslak reviewed Oct 28, 2024
View reviewed changes
pkg/provider/provider.go Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Integration tests failure for 075be188551fe6d40f335dab2bdfd11f2fc8744a

@github-actions GitHub Actions
Copy link

Integration tests failure for 9f15217972c53739d32ee705d6698d6bfc749f79

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review October 28, 2024 17:08
sfc-gh-asawicki
sfc-gh-asawicki requested changes Oct 28, 2024
View reviewed changes
pkg/helpers/special_values.go Outdated
@@ -0,0 +1,60 @@
package helpers
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move away from this package (use internal/*); we will do with a bigger cleanup but as the general rule, but if we are moving them now, let's place them more correctly, e.g. we have:

  • internal/provider
  • internal/provider/docs
    maybe we could add internal/provider/validators for validations (then usage is nicer - validators.ValidateXyz and we won't have to move it again), special values may reside it internal/provider and helpers/doc can go to internal/provider/docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

pkg/provider/provider_acceptance_test.go
},
Steps: []resource.TestStep{
{
ExternalProviders: map[string]resource.ExternalProvider{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you had in the doc a proposal to extract short func to setup such tests, so maybe let's add this method and use it in the new tests (even without using it in all the places)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do this in the next pr.

@github-actions GitHub Actions
Copy link

Integration tests success for 4c95204e83774e1396e0c92ef46b44987a2650cb

@github-actions GitHub Actions
Copy link

Integration tests failure for fdb784e81346e417ec44c078d31588bfcb7bd295

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review October 29, 2024 18:25
@sfc-gh-jmichalak sfc-gh-jmichalak merged commit fd6af43 into main Oct 30, 2024
8 of 9 checks passed
@sfc-gh-jmichalak sfc-gh-jmichalak deleted the config-rework branch October 30, 2024 09:33
@snowflake-release-please snowflake-release-please bot mentioned this pull request Oct 30, 2024
Merged
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
@snowflake-release-please
chore(main): release 0.98.0 (#3137)
003999e 
##
[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
Reviewers

@sfc-gh-asawicki sfc-gh-asawicki sfc-gh-asawicki approved these changes

@sfc-gh-jcieslak sfc-gh-jcieslak sfc-gh-jcieslak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-jmichalak @sfc-gh-jcieslak @sfc-gh-asawicki