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

compile --no-inject-ephemeral-ctes flag #8482

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Conversation

benmosher
Copy link
Contributor

@benmosher benmosher commented Aug 23, 2023

...for inline compilation of models during linting only.

resolves #8480

Problem

Injection of ephemeral model CTEs breaks SQLFluff linting in Cloud.

Solution

Optionally suppressing the injection of CTEs (intentionally added to the compile command only!) enables SQLFluff linting of models with direct ephemeral model references.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • Revalidate in an IDE session

@cla-bot
Copy link

cla-bot bot commented Aug 23, 2023

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @benmosher

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (4ec87a0) 86.36% compared to head (1235de1) 86.37%.
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8482      +/-   ##
==========================================
+ Coverage   86.36%   86.37%   +0.01%     
==========================================
  Files         174      174              
  Lines       25575    25591      +16     
==========================================
+ Hits        22087    22104      +17     
+ Misses       3488     3487       -1     
Flag Coverage Δ
integration 83.18% <100.00%> (-0.03%) ⬇️
unit 65.04% <50.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
core/dbt/cli/main.py 98.69% <100.00%> (+<0.01%) ⬆️
core/dbt/cli/params.py 100.00% <100.00%> (ø)
core/dbt/compilation.py 96.19% <100.00%> (+0.02%) ⬆️

... and 33 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbeatty10
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Aug 24, 2023
@cla-bot
Copy link

cla-bot bot commented Aug 24, 2023

The cla-bot has been summoned, and re-checked this pull request!

@MichelleArk MichelleArk self-requested a review August 30, 2023 22:51
@benmosher
Copy link
Contributor Author

@MichelleArk -- I see your 👀; I left this in draft because the last checkbox is unchecked but it's otherwise ready to go AFAIK. anything else I should do?

core/dbt/cli/params.py Outdated Show resolved Hide resolved
core/dbt/compilation.py Outdated Show resolved Hide resolved
@MichelleArk
Copy link
Contributor

@benmosher - with regards to that last checkbox, @graciegoheen will be helping out with this review from the Product perspective so we can get approval on that front before merging :)

@MichelleArk MichelleArk added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Sep 5, 2023
@graciegoheen
Copy link
Contributor

Let's hide the flag from the CLI --help text, since I think it will be confusing clutter for 99% of end users. Same as we did for #7911

@benmosher
Copy link
Contributor Author

@graciegoheen agreed! I believe I've hidden it via this hidden=True 👍

@benmosher benmosher changed the title compile --suppress-ephemeral-ctes flag compile --no-inject-ephemeral-ctes flag Sep 6, 2023
@benmosher benmosher marked this pull request as ready for review September 6, 2023 17:31
@benmosher benmosher requested a review from a team as a code owner September 6, 2023 17:31
@benmosher
Copy link
Contributor Author

I'd like to revalidate this in the ide-server before merging after changing the flag name. Should be fine since the test still passes but just want to be sure!

@benmosher
Copy link
Contributor Author

Revalidation complete 👍

@MichelleArk
Copy link
Contributor

🚢 🇮🇹

@benmosher
Copy link
Contributor Author

@MichelleArk I think you need to push the button -- I'm not authorized to merge to Core 😅

@MichelleArk MichelleArk merged commit e24a952 into main Sep 7, 2023
@MichelleArk MichelleArk deleted the bmo/no-ephemeral-ctes branch September 7, 2023 14:13
github-actions bot pushed a commit that referenced this pull request Sep 11, 2023
benmosher added a commit that referenced this pull request Sep 12, 2023
benmosher added a commit that referenced this pull request Sep 12, 2023
emmyoop pushed a commit that referenced this pull request Sep 12, 2023
(cherry picked from commit e24a952)

Co-authored-by: Ben Mosher <[email protected]>
@aranke aranke mentioned this pull request Jul 12, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6.latest cla:yes Impact: Exp ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-3023] [Feature] compile --suppress-ephemeral-ctes
4 participants