-
Notifications
You must be signed in to change notification settings - Fork 427
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: Add managed account to the SDK #2357
Conversation
Integration tests failure for 916c8218016f939b1d3b418bb30a696fe357f7fa |
916c821
to
dac416d
Compare
Integration tests failure for dac416d5bb1bb0b7abc1b2f37419212e10f370e1 |
g.NewQueryStruct("CreateManagedAccountParams"). | ||
TextAssignment("ADMIN_NAME", g.ParameterOptions().SingleQuotes().Required()). | ||
TextAssignment("ADMIN_PASSWORD", g.ParameterOptions().SingleQuotes().Required()). | ||
PredefinedQueryStructField("typeProvider", "string", g.StaticOptions().SQL("TYPE = READER")). |
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.
should this be an enum rather than hardcoding? currently the only type supported is READER, but that could change in the future.
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.
It can change in the future, but it's currently hardcoded in the docs. We can change it without problems later (if needed - because it may never change).
} | ||
} | ||
|
||
createManagedAccountBasicRequest := func(t *testing.T) *sdk.CreateManagedAccountRequest { |
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.
we should have a discussion on what the structure for these kinds of test request helpers should be. i think there are a few different patterns right now. also there's the helpers_test.go file in the integration test folder, I think we should refactor that as well.
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.
We have already an issue for the helper test (SNOW-936093). We will have the discussion there. You are right we should unify the current approaches.
Add managed account to the SDK:
ADMIN_NAME
References