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

chore: Continue random ids rework #2819

Merged
merged 22 commits into from
May 20, 2024
Merged

chore: Continue random ids rework #2819

merged 22 commits into from
May 20, 2024

Conversation

sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki commented May 17, 2024 •
edited
Loading

Continuation of #2747 (topics connected with NewAccountObjectIdentifier):

  • alterMaterializedViewQueryExternally - moved to test helper
  • checkDatabaseAndSchemaDataRetentionTime - id part reworked, method left for assertions introduction
  • checkDatabaseSchemaAndTableDataRetentionTime - id part reworked, method left for assertions introduction
  • unsafe_execute_acceptance_test.go - id part reworked, some methods left for assertions introduction
  • alterViewOwnershipExternally - moved to test helper
  • check app_role_1 - test role name extracted and parametrized in tests
  • check hardcoded filter_by_role, stproc1, sp_pi, filterByRole, records - added to procedure rework
  • ask @sfc-gh-jcieslak about sdk.NewAccountObjectIdentifier("S3_STORAGE_INTEGRATION"), sdk.NewAccountObjectIdentifier("GCP_STORAGE_INTEGRATION"), sdk.NewAccountObjectIdentifier("AZURE_STORAGE_INTEGRATION") - precreated integrations extracted
  • extract common helpers for sdk.NewAccountObjectIdentifier("does_not_exist") and similar - extracted
  • Fixed setup for multiple acceptance tests (to use random ids not random strings)

Still left (connected with NewAccountObjectIdentifier):

  • CreateDatabaseWithName (and other similar helpers)
  • check setup_test.go

Additional changes:

  • changed return type for CurrentRole and CurrentUser to ids
  • Fixed ShowByID for application roles
  • Added FAQ entry about possible identifier error
  • Introduced DatabaseId() and SchemaId() helpers to identifiers and convenience methods to use them
  • Introduced some additional placeholders (like role ids for ACCOUNTADMIN and ORGADMIN)
  • Moved check destroy to a common place

@sfc-gh-asawicki
Add identifier interface conversion to FAQ
60e5aa6
@sfc-gh-asawicki
Change current role helper response to the identifier
e11cd46
@sfc-gh-asawicki
Change string to identifier for database existence checker
9e4f942
@sfc-gh-asawicki
Move checking revoking privileges from share to other check destroy f…
ee0704b 
…unctions
@sfc-gh-asawicki
Introduce materialized view helper and use it
244a03d
@sfc-gh-asawicki
Clean materialized view acceptance tests a bit
1f99de8
@sfc-gh-asawicki
Introduce DatabaseId for DatabaseObjectIdentifier
b5eed4b
@sfc-gh-asawicki
Use Introduced DatabaseId and SchemaId
a7390c2
@sfc-gh-asawicki
Move helpers from view acceptance test
37758b5
@sfc-gh-asawicki
Extract precreated storage integration ids
ef8038c
@sfc-gh-asawicki
Simplify id creation for schema
45461fb
@sfc-gh-asawicki
Simplify id creation for table
6249726
@sfc-gh-asawicki
Use identifiers directly in unsafe execute tests
3b03063
@sfc-gh-asawicki
Change return type for current user helper
46e46a0
@sfc-gh-asawicki
Add TODO for using context client
6ecbb32
@sfc-gh-asawicki
Remove new id from resources package
f037fb5
@sfc-gh-asawicki
Finish NewAccountObjectIdentifier topics
f4612e4
@sfc-gh-asawicki
Add TODOs for procedure hardcoded identifiers
daef1cd
@sfc-gh-asawicki
Run make pre-push
2b7c8fa
@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review May 17, 2024 13:30
@github-actions GitHub Actions
Copy link

Integration tests failure for 2b7c8fa61eea2a35d8378fb62e88da489f63406c

@github-actions GitHub Actions
Copy link

Integration tests failure for 811cd292fa59158a9042cddd5d965cfa80e08e5d

@github-actions GitHub Actions
Copy link

Integration tests success for 5f2c544fade76d20900a02fe53c096a7666e984f

sfc-gh-jcieslak
sfc-gh-jcieslak reviewed May 20, 2024
View reviewed changes
CREATING_ISSUES.md Outdated
- AccountObjectIdentifier - `<name>`
- DatabaseObjectIdentifier - `<database>.<name>`
- SchemaObjectIdentifier - `<database>.<schema>.<name>`
- NewTableColumnIdentifier - `<database>.<schema>.<table>.<name>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it a constructor name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, fixed

pkg/acceptance/helpers/ids_generator.go Show resolved Hide resolved
pkg/sdk/testint/streamlits_integration_test.go Show resolved Hide resolved
pkg/acceptance/helpers/role_client.go Outdated
@@ -114,6 +116,31 @@ func (c *RoleClient) GrantOwnershipOnAccountObject(t *testing.T, roleId sdk.Acco
require.NoError(t, err)
}

// TODO: move later to grants client
func (c *RoleClient) RevokeCurrentGrantsFromSchemaObject(t *testing.T, roleId sdk.AccountObjectIdentifier, objectId sdk.SchemaObjectIdentifier, objectType sdk.ObjectType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename it to MoveOwnershipAndRevokeCurrentGrantsFromSchemaObject? It's a long name, but it mentions that it's calling grant ownership rather than revoke privileges from.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the signature to func (c *RoleClient) GrantOwnershipOnSchemaObject(t *testing.T, roleId sdk.AccountObjectIdentifier, objectId sdk.SchemaObjectIdentifier, objectType sdk.ObjectType, outboundPrivileges sdk.OwnershipCurrentGrantsOutboundPrivileges) instead

sfc-gh-jmichalak
sfc-gh-jmichalak approved these changes May 20, 2024
View reviewed changes
pkg/resources/schema.go
@@ -107,7 +107,7 @@ func ReadSchema(d *schema.ResourceData, meta interface{}) error {
ctx := context.Background()
id := helpers.DecodeSnowflakeID(d.Id()).(sdk.DatabaseObjectIdentifier)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this cast could be done in a helper function, we have a switch inside anyway, so we could return proper type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be refined during the identifiers rework, this is the current approach.

Also, the DecodeSnowflakeId function is currently returning the interface (and internally the values are cast), so the cast here is necessary with the current implementation.

@github-actions GitHub Actions
Copy link

Integration tests failure for c07fea1b59c5609c49ecf4cbd61d24f2385aa7e9

@sfc-gh-asawicki sfc-gh-asawicki merged commit f20940c into main May 20, 2024
8 of 9 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the random-ids-rework-continuation branch May 20, 2024 09:39
@snowflake-release-please snowflake-release-please bot mentioned this pull request May 20, 2024
Merged
sfc-gh-jcieslak pushed a commit that referenced this pull request Jun 6, 2024
@snowflake-release-please
chore(main): release 0.92.0 (#2824)
5df6d3d 
🤖 I have created a release *beep* *boop*
---


##
[0.92.0](v0.91.0...v0.92.0)
(2024-06-06)


### 🎉 **What's new:**

* Add Api Authentication security integration to sdk
([#2840](#2840))
([57a07ee](57a07ee))
* Add External Oauth security integration to sdk
([#2835](#2835))
([82d1c09](82d1c09))
* add network rules
([#2746](#2746))
([c79fa29](c79fa29))
* Add SCIM and SAML2 security integrations to sdk
([#2799](#2799))
([1312ff1](1312ff1))
* Add Snowflake Oauth security integration to sdk
([#2830](#2830))
([b576f29](b576f29))
* Database resource v1 readiness
([#2834](#2834))
([30fe136](30fe136))
* Database SDK upgrade
([#2814](#2814))
([750fe37](750fe37))


### 🔧 **Misc**

* accept non-pointer values in the generated builder methods
([#2816](#2816))
([c29fbf1](c29fbf1))
* Add a script for creating labels
([#2778](#2778))
([ce0fbad](ce0fbad))
* Adjust before 0.92.0
([#2857](#2857))
([0598656](0598656))
* Continue random ids rework
([#2819](#2819))
([f20940c](f20940c))
* Random ids rework part3
([#2833](#2833))
([36ead85](36ead85))
* Random ids rework part4
([#2837](#2837))
([64518a3](64518a3))
* Update issue for table and warehouse redesign state
([#2845](#2845))
([149e55e](149e55e))


### 🐛 **Bug fixes:**

* Fix failing integration tests
([#2832](#2832))
([2e2ca6c](2e2ca6c))
* Fix QUOTED_IDENTIFIERS_IGNORE_CASE parameter test
([#2841](#2841))
([92ad1d3](92ad1d3))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

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-jmichalak sfc-gh-jmichalak sfc-gh-jmichalak 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-asawicki @sfc-gh-jcieslak @sfc-gh-jmichalak