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: Mark V1 ready preview resources #3218

Merged
merged 4 commits into from
Nov 26, 2024
Merged

feat: Mark V1 ready preview resources #3218

merged 4 commits into from
Nov 26, 2024

Conversation

sfc-gh-jmichalak
Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak commented Nov 22, 2024 •
edited
Loading

  • mark preview resources
  • add a separate flag for enabling all of the resources to simplify tests

Test Plan

  • acceptance tests
  • …

References

sfc-gh-asawicki
sfc-gh-asawicki requested changes Nov 22, 2024
View reviewed changes
pkg/datasources/current_account.go Outdated
@@ -41,6 +42,10 @@ func CurrentAccount() *schema.Resource {
// ReadCurrentAccount read the current snowflake account information.
func ReadCurrentAccount(d *schema.ResourceData, meta interface{}) error {
client := meta.(*provider.Context).Client
enabled := meta.(*provider.Context).EnabledFeatures
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do a few things:

  1. Extract previewfeatures pkg inside provider.
  2. Move the list of preview features there.
  3. Make the list closed (the same way as for resources.Resource)
  4. Make EnabledFeatures type list of the type above instead of string.
  5. Print all available features in the docs.
  6. To the extracted package add func EnsurePreviewFeatureEnabled(previewfeatures.CurrentAccount, meta.(*provider.Context).EnabledFeatures) that encapsulates the logic and return error and here just diag.FromErr(err)
  7. (question) why not set in tf config?
  8. Add validation to check if provided values are only the possible values.

WDYT @sfc-gh-jmichalak @sfc-gh-jcieslak ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LGTM, but we already have a validation on the provider level. Do you have something else in mind?

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree + I think my wrapper function could come in clutch here, because instead of going through all the resources and data sources you can just add the check in the common function. (let's discuss this on Monday)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we extract them as this fancy closed list then we need conversion from string to this new type, preferably inside this new pkg, so this validation in provider changes, sorry for not being precise

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. Ad.7 - it's supposed to be Set, I was just playing around with it.

pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider_acceptance_test.go Show resolved Hide resolved
sfc-gh-jcieslak
sfc-gh-jcieslak reviewed Nov 22, 2024
View reviewed changes
pkg/datasources/current_account.go Outdated
@@ -41,6 +42,10 @@ func CurrentAccount() *schema.Resource {
// ReadCurrentAccount read the current snowflake account information.
func ReadCurrentAccount(d *schema.ResourceData, meta interface{}) error {
client := meta.(*provider.Context).Client
enabled := meta.(*provider.Context).EnabledFeatures
Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree + I think my wrapper function could come in clutch here, because instead of going through all the resources and data sources you can just add the check in the common function. (let's discuss this on Monday)

pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Integration tests failure for 0c97cb97b09ac78111b2bf4351682f39fb462b33

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review November 25, 2024 14:40
sfc-gh-jcieslak
sfc-gh-jcieslak reviewed Nov 25, 2024
View reviewed changes
pkg/internal/tracking/context.go
@@ -8,7 +8,7 @@ import (
)

const (
ProviderVersion string = "v0.99.0" // TODO(SNOW-1814934): Currently hardcoded, make it computed
ProviderVersion string = "v1.0.0-rc.1" // TODO(SNOW-1814934): Currently hardcoded, make it computed
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it v1.0.0-rc1

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 initially did, but realized that in semantic versioning they use . here: https://semver.org/

Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. (https://semver.org/#spec-item-9) - so in our case I would say that rc1 is and identifier (not two identifiers rc and 1), so I agree with @sfc-gh-jcieslak

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'll change this in the next pr.

MIGRATION_GUIDE.md Show resolved Hide resolved
Base automatically changed from v1-ready-remove-deprecated to v1-ready November 25, 2024 15:15
@github-actions GitHub Actions
Copy link

Integration tests failure for f6bdff938ce4f205b577fdf6920453521ed3104e

@sfc-gh-jmichalak sfc-gh-jmichalak force-pushed the v1-ready-preview-resources branch from f6bdff9 to f9920d7 Compare November 25, 2024 16:34
@github-actions GitHub Actions
Copy link

Integration tests failure for f9920d77eb6b207583a8380240c2f31b0df678e8

@sfc-gh-jmichalak sfc-gh-jmichalak marked this pull request as ready for review November 26, 2024 09:48
@sfc-gh-jmichalak sfc-gh-jmichalak merged commit e0cfcef into v1-ready Nov 26, 2024
8 of 9 checks passed
@sfc-gh-jmichalak sfc-gh-jmichalak deleted the v1-ready-preview-resources branch November 26, 2024 10:18
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