-
Notifications
You must be signed in to change notification settings - Fork 422
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
chore: Basic object tracking #3205
Conversation
Integration tests failure for 43ec04e4c1571f7163a9606c4d04240aa5216bef |
Integration tests success for 19ed10a89f1543b36bf2a8eb5fd3ec98c2591b38 |
return Metadata{}, fmt.Errorf("failed to parse metadata from sql, incorrect number of parts, expected: 2, got: %d", len(parts)) | ||
} | ||
var metadata Metadata | ||
if err := json.Unmarshal([]byte(strings.TrimSpace(parts[1])), &metadata); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think that Unmarshal doesn't require the data to be trimmed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave it just to be sure (I'm trimming the space after usage tracking prefix)
log.Printf("[ERROR] failed to append metadata tracking: %v\n", err) | ||
return sql | ||
} | ||
return newSql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also log here if there's no metadata in the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if that's necessary for now (could be added later) because it would log this message a lot
# Conflicts: # pkg/provider/resources/resources.go
@@ -277,7 +277,8 @@ func (c *Client) exec(ctx context.Context, sql string) (sql.Result, error) { | |||
log.Printf("[DEBUG] sql-conn-exec-dry: %v\n", sql) | |||
return nil, nil | |||
} | |||
ctx = context.WithValue(ctx, snowflakeAccountLocatorContextKey, c.accountLocator) | |||
ctx = context.WithValue(ctx, SnowflakeAccountLocatorContextKey, c.accountLocator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in follow also use struct{} here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #3214
Integration tests failure for cdbf3fefd251c23dd451aa9e85a2298a80966db0 |
Integration tests failure for b53359f77b7aba9d2194b9bbda958420bc64566d |
follow up to #3205 ## Changes - applied comments from the previous pr (added test in resource and account locator key was changed to `struct{}`, etc.) - applied tracking wrappers to every resource that has implemented context method (in old ones we create context ourselves; I can go and add the context there too), also imports implemented as passthrough were skipped. - added missing resource names to `resource` type - added entry to our v1 guidelines to add wrapper around terraform functions - modified test client for information schema a bit ## Coming up - data sources (after pr that adds data source names similar to the `resrouces.Resource` interface)
🤖 I have created a release *beep* *boop* --- ## [0.99.0](v0.98.0...v0.99.0) (2024-11-26) ### 🎉 **What's new:** * Add tags data source ([#3211](#3211)) ([8907d9d](8907d9d)) * Tag resource v1 ([#3197](#3197)) ([77b3bf0](77b3bf0)) * Tasks v1 readiness ([#3222](#3222)) ([e2284d9](e2284d9)) ### 🔧 **Misc** * Add support for usage tracking to data sources ([#3224](#3224)) ([8210bb8](8210bb8)) * Add usage tracking for the rest of the resources and fix views ([#3223](#3223)) ([231f653](231f653)) * Basic object tracking ([#3205](#3205)) ([1f0dc94](1f0dc94)) * basic object tracking part 2 ([#3214](#3214)) ([e44f2e1](e44f2e1)) * Improve tags integration tests ([#3193](#3193)) ([7736e0a](7736e0a)) * parser and secret tests ([#3192](#3192)) ([5ec9c86](5ec9c86)) * Storage integration with custom protocol ([#3213](#3213)) ([a3a44ae](a3a44ae)) * Unskip auth config tests ([#3180](#3180)) ([46ab142](46ab142)) ### 🐛 **Bug fixes:** * Small fixes and adjustments ([#3226](#3226)) ([9f67457](9f67457)) --- 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>
Changes
Next pr