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.

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

fix: Update Snowflake Terraform provider source to Snowflake-Labs #400

Merged
merged 12 commits into from
Jun 7, 2022

Conversation

jjb007
Copy link
Contributor

@jjb007 jjb007 commented Jun 6, 2022

Summary

With the source of the Snowflake Terraform provider transitioned from CZI to Snowflake, hard-coded mentions need to be updated accordingly.

Also fixed code to validate CI configuration from YAML to handle string scalars as well as array for $.jobs.test.strategy.matrix.module.

Test Plan

cd scripts/snowflake_generate_grant_all
make run

References

ONCALL-65

@jjb007 jjb007 requested review from jakeyheath and alldoami June 6, 2022 20:56
@jakeyheath
Copy link
Contributor

I ran go mod tidy to update the deps properly. Let's see if that fixes some of the test failures.

Copy link
Contributor Author

@jjb007 jjb007 left a comment

Choose a reason for hiding this comment

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

Also, is it concerning that running scripts/snowflake_generate_grant_all generated all these other modules?

Comment on lines +38 to +43
if _, ok := matrix["module"].([]interface{}); ok {
for _, module := range matrix["module"].([]interface{}) {
modules.Add(module.(string))
}
} else {
modules.Add(matrix["module"].(string))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the code change I made. Let me know if this is appropriate/idiomatic for Go and/or the program itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the infra team back then was really into code generation so I feel like this not surprising. I think the idiomatic way of doing this parsing though would be to make a type for the github action yaml and call something like err = yaml.Unmarshal(yamlFile, &config) instead of doing type casting. However, I am not sure it is worth the half an hour to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In short, I think this is fine and good 👍🏻

@alldoami
Copy link
Contributor

alldoami commented Jun 7, 2022

yayyy tests are passing!

@jjb007 jjb007 marked this pull request as ready for review June 7, 2022 17:17
@jjb007 jjb007 requested a review from a team as a code owner June 7, 2022 17:17
@jjb007 jjb007 merged commit fb7b329 into main Jun 7, 2022
@jjb007 jjb007 deleted the jbartolome/ONCALL-65/1 branch June 7, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants