-
-
Notifications
You must be signed in to change notification settings - Fork 686
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
sqlfluff lint: Add "github-annotation" format #1198
sqlfluff lint: Add "github-annotation" format #1198
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1198 +/- ##
==========================================
+ Coverage 96.07% 96.10% +0.02%
==========================================
Files 124 124
Lines 8469 8477 +8
==========================================
+ Hits 8137 8147 +10
+ Misses 332 330 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@GClunies: Would it make sense to update the existing SQLFluff GitHub Action to leverage this? Are any changes required in order for this to work? |
elif serialize == "yaml": | ||
result = yaml.safe_load(result.output) | ||
assert len(result) == 2 | ||
elif serialize == "github-annotation": | ||
result = json.loads(result.output) |
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.
Let's add a unit test that asserts the full response?
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!
Nice! |
Sorry for the late catch up here! Just read through the original slack thread and GitHub Issue, I really like the idea proposed by @emily-hawkins and looks like this PR is well on its way to a solution @barrywhart! From my brief review it seems like we would just need to modify the end of the GitHub workflow to be something like... - name: Lint dbt models
if: steps.get_files_to_lint.outputs.lintees != ''
shell: bash -l {0}
run: |
sqlfluff lint --format github-annotation --annotation-level failure ${{ steps.get_files_to_lint.outputs.lintees }}
- name: Annotate PR
uses: yuzutech/annotations-action@v0.1.0
with:
repo-token: "${{ secrets.GITHUB_TOKEN }}"
title: 'SQLFluff'
input: './annotations.json' @emily-hawkins, have you had a chance to test this out on your project? |
Not yet! I was going to test it out once the params for annotation-level are added in |
@emily-hawkins: Sorry, I didn't have time to work on this today. Will try and get to it tomorrow. |
@emily-hawkins: I added the |
@GClunies: A question about your suggested steps for the GitHub Action: is this going to redirect Note that without redirection, running |
thank you!! i will test this out today |
i got the sqlfluff parts to run on my end, but getting an issue now on the annotate action, which i put in an issue for yuzutech/annotations-action#36 |
I got this to work!! there is something off in the newest version so downgrading the annotation action to 0.2.1 fixed it! |
This is so exciting! Would love to see a write-up if you can find the time! |
for documentation, i can show what my workflows file looks like for 1) testing this PR and 2) how it will look when this is in production
|
This is awesome @emily-hawkins and @barrywhart! |
the person who owns the annotation action got back to me about why it might not be working in v 0.3.0
|
@emily-hawkins: I updated the output format to include the |
@emily-hawkins, @GClunies: I initially thought about documenting GitHub Actions and annotations in this PR, but I realize now it would be better to include it in https://github.com/sqlfluff/sqlfluff-github-actions. What do y'all think? |
yes! ill test this out today |
just tried it with v0.3.0 with these changes and it's working 🎉 |
Thanks so much, @emily-hawkins! I'm going to go ahead and merge this. It will be cool to get this documented in https://github.com/sqlfluff/sqlfluff-github-actions at some point. |
@barrywhart, it seems as simple as just modifying the workflow here so that the end is: - name: Lint dbt models
id: sqlfluff_json
if: steps.get_files_to_lint.outputs.lintees != ''
shell: bash -l {0}
run: |
sqlfluff lint --format github-annotation --annotation-level failure --nofail ${{ steps.get_files_to_lint.outputs.lintees }} > annotations.json
- name: Annotate SQLFluff Linting in PR
uses: yuzutech/annotations-action@v0.2.1
with:
repo-token: "${{ secrets.GITHUB_TOKEN }}"
title: "SQLFluff Lint"
input: "./annotations.json" Not sure much else needs to be done in terms of docs. This will "just work" from what I see. Just about to go on PTO, so classic Friday rush to tie up loose end before I leave 😅 . Will PR on the plane! |
Hi team, I'm seeing what appears to be a related issue to this when setting up my own project. The issue I'm having is that the annotations step is blocked by the ==== readout . . . line at the top of the output file from the linting step. I was able to move forward by adding this very ugly
Am I missing a simpler way to do this? |
Resolves #1190