-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add ability to configure linting on CI jobs #338
Conversation
@@ -9,15 +9,15 @@ import ( | |||
) | |||
|
|||
type JobTrigger struct { | |||
Github_Webhook bool `json:"github_webhook"` |
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.
Did you update all of those by hand or used some tool for it? I have been writing new code with CamlCase but didn't touch most of the code that was written before I got involved.
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 used IntelliJ (GoLand) to do the rename, so it updates all references as well.
Name string `json:"name"` | ||
DbtVersion *string `json:"dbt_version"` |
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.
Same here. Did you sort them by alphabetical order or are you using a tool for it? Historically, I have been trying to group new fields together but it also makes sense to have those in alphabetical order.
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.
The ordering I did by hand using IntelliJ to move the lines up/down (cmd-shift up/down).
resource.TestCheckResourceAttr("dbtcloud_job.ci_job", "name", jobName), | ||
resource.TestCheckResourceAttr("dbtcloud_job.ci_job", "run_lint", "false"), | ||
resource.TestCheckResourceAttr("dbtcloud_job.ci_job", "errors_on_lint_failure", "true"), |
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.
Maybe a bigger discussion for the future, but I read somewhere that we actually might not need to check attributes that are not Computed
. The testing framework actually checks that all the non Computed values are OK based on the config (e.g. it makes sure that a plan
after an apply
would not raise any diff). I know that this is not the way that the provider has been written so far though.
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.
That makes sense! Let me read up on it and maybe I can remove those!
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.
Thanks @csquire
I left some comments but more for my own understanding.
LGTM
The failing test is with another resource and might be related to a transient issue with Tableau at that time, it is not related to your change.
Let's make sure that we mention the bugfix in the changelog when we do the new release.
Resolves #310
Adds fields to the
dbtcloud_job
resource in support of linting. Also just a bit of code cleanup by switching some internal job-related struct fields to camel-case instead of snake-case.run_lint
: Whether the CI job should lint SQL changes. Defaults tofalse
.errors_on_lint_failure
: Whether the CI job should fail when a lint error is found. Only used whenrun_lint
is set totrue
. Defaults totrue
.Note that the defaults match dbt Cloud's default behavior.
Example: