feat: add command to run apply defer config #1612
Conversation
WalkthroughThis pull request introduces a new command Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VS as VSCodeCommands
participant DC as dbtProjectContainer
participant P as Project
U->>VS: Trigger command (dbtPowerUser.applyDeferConfig)
VS->>DC: getProjects()
DC-->>VS: [Project1, Project2, ...]
par Concurrently Apply Defer Config
VS->>P: applyDeferConfig()
Note over P: Process deferred configuration
end
Note over VS: Await completion using Promise.all
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You could try using a tool like Cursor or aider to try develop this feature. |
|
Can you elaborate on this:
why is the config not enough? |
|
Our issue is that when we set up the github codespace, we predefine the config relating to the defer. Once this manifest has been downloaded, then I need to somehow tell dbt Power User that the manifest has been added/changed, since it doesn't seem to realize this on its own. So that's why I need this command to be available, so that my task can simply trigger that command as soon as the manifest is added/updated. |
|
Can you not add this logic to the run command of your docker container before invocation of vscode? |
|
The download of the production manifest needs to be done following user authentication to GCP (using oauth), as it uses those credentials. And secondly, the user might have a long running codespace and would need to update the manifest from time to time. But in any case, I don't see the downside of having this action available as a command. |
|
Of course, the best thing would be if the extension simply always re-read the manifest at the given path, rather than storing it in cache somewhere, as it seems to be doing now. If we only had to update the file and could then be sure that the updated file would be used, then all would be splendid. |
Co-authored-by: Michiel De Smet <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to cdb82e1 in 3 minutes and 2 seconds
More details
- Looked at
31lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/commands/index.ts:800
- Draft comment:
The new command callback uses 'await' but is not declared as async. Add the async keyword to the callback function. - Reason this comment was not posted:
Marked as duplicate.
2. src/commands/index.ts:858
- Draft comment:
Typo: In the log message at line 858, change "File doesn't exists at location" to "File doesn't exist at location" for correct grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/commands/index.ts:644
- Draft comment:
Consider capitalizing 'css' to 'CSS' in the string for consistency in the decoration options (line 644). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_r9KOND5OTHazEdXv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/index.ts (1)
800-803: Consider adding error handling and user feedbackThe implementation applies configurations across all projects but doesn't provide any feedback or handle potential errors.
Consider improving the implementation with error handling and user feedback:
- commands.registerCommand("dbtPowerUser.applyDeferConfig", () => { + commands.registerCommand("dbtPowerUser.applyDeferConfig", async () => { + try { const projects = this.dbtProjectContainer.getProjects(); - await Promise.all(projects.map((project) => project.applyDeferConfig())); + if (projects.length === 0) { + window.showInformationMessage("No dbt projects found to apply defer configuration"); + return; + } + + await Promise.all(projects.map((project) => project.applyDeferConfig())); + window.showInformationMessage("Successfully applied defer configuration to all projects"); + } catch (error) { + this.dbtTerminal.error( + "applyDeferConfig", + "An error occurred while applying defer configuration", + error + ); + window.showErrorMessage( + "An error occurred while applying defer configuration: " + error + ); + } }),🧰 Tools
🪛 Biome (1.9.4)
[error] 802-802:
awaitis only allowed within async functions and at the top levels of modules.(parse)
🪛 ESLint
[error] 802-802: Replace
projects.map((project)·=>·project.applyDeferConfig())with⏎··········projects.map((project)·=>·project.applyDeferConfig()),⏎········(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/commands/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/commands/index.ts
[error] 802-802: await is only allowed within async functions and at the top levels of modules.
(parse)
🪛 ESLint
src/commands/index.ts
[error] 802-802: Replace projects.map((project)·=>·project.applyDeferConfig()) with ⏎··········projects.map((project)·=>·project.applyDeferConfig()),⏎········
(prettier/prettier)
🔇 Additional comments (1)
package.json (1)
757-762: LGTM! The command definition is correctly added.The new command
dbtPowerUser.applyDeferConfigis properly defined with appropriate category and title. This aligns well with the PR objective of automating the "defer to production" feature activation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commands/index.ts (1)
800-803: Good implementation of the defer configuration command.The command correctly retrieves all projects and applies the defer configuration concurrently using Promise.all. The async/await syntax is properly implemented.
However, consider adding error handling to gracefully manage potential failures during the operation.
commands.registerCommand("dbtPowerUser.applyDeferConfig", async () => { const projects = this.dbtProjectContainer.getProjects(); - await Promise.all(projects.map((project) => project.applyDeferConfig())); + try { + await Promise.all(projects.map((project) => project.applyDeferConfig())); + window.showInformationMessage("Successfully applied defer configuration to all projects"); + } catch (error) { + this.dbtTerminal.error("applyDeferConfig", "Failed to apply defer configuration", error); + window.showErrorMessage(`Failed to apply defer configuration: ${error}`); + } }),🧰 Tools
🪛 ESLint
[error] 802-802: Replace
projects.map((project)·=>·project.applyDeferConfig())with⏎··········projects.map((project)·=>·project.applyDeferConfig()),⏎········(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands/index.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/commands/index.ts
[error] 802-802: Replace projects.map((project)·=>·project.applyDeferConfig()) with ⏎··········projects.map((project)·=>·project.applyDeferConfig()),⏎········
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commands/index.ts (1)
800-809: Well-implemented applyDeferConfig command!The implementation effectively addresses the goal of automating the "defer to production" feature activation. It efficiently applies configurations to all projects concurrently, includes proper error handling, and provides clear feedback to users.
Consider addressing the ESLint formatting issues by applying this diff:
commands.registerCommand("dbtPowerUser.applyDeferConfig", async () => { const projects = this.dbtProjectContainer.getProjects(); try { - await Promise.all(projects.map((project) => project.applyDeferConfig())); - window.showInformationMessage("Applied defer configuration"); + await Promise.all( + projects.map((project) => project.applyDeferConfig()), + ); + window.showInformationMessage("Applied defer configuration"); } catch (error) { - this.dbtTerminal.error("applyDeferConfig", "Failed to apply defer configuration", error); - window.showErrorMessage(`Failed to apply defer configuration: ${error}`); + this.dbtTerminal.error( + "applyDeferConfig", + "Failed to apply defer configuration", + error, + ); + window.showErrorMessage( + `Failed to apply defer configuration: ${error}`, + ); } }),🧰 Tools
🪛 ESLint
[error] 803-803: Replace
projects.map((project)·=>·project.applyDeferConfig())with⏎············projects.map((project)·=>·project.applyDeferConfig()),⏎··········(prettier/prettier)
[error] 806-806: Replace
"applyDeferConfig",·"Failed·to·apply·defer·configuration",·errorwith⏎············"applyDeferConfig",⏎············"Failed·to·apply·defer·configuration",⏎············error,⏎··········(prettier/prettier)
[error] 807-807: Replace
Failed·to·apply·defer·configuration:·${error}with⏎············Failed·to·apply·defer·configuration:·${error},⏎··········(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/commands/index.ts (1)
src/dbt_client/dbtTerminal.ts (1) (1)
error(105-125)
🪛 ESLint
src/commands/index.ts
[error] 803-803: Replace projects.map((project)·=>·project.applyDeferConfig()) with ⏎············projects.map((project)·=>·project.applyDeferConfig()),⏎··········
(prettier/prettier)
[error] 806-806: Replace "applyDeferConfig",·"Failed·to·apply·defer·configuration",·error with ⏎············"applyDeferConfig",⏎············"Failed·to·apply·defer·configuration",⏎············error,⏎··········
(prettier/prettier)
[error] 807-807: Replace Failed·to·apply·defer·configuration:·${error} with ⏎············Failed·to·apply·defer·configuration:·${error},⏎··········
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/commands/index.ts (1)
800-817: Excellent implementation of the apply defer config command.The code successfully implements a new command that applies the deferred configuration to all projects. The approach of using Promise.all for concurrent execution is efficient, and the implementation includes proper error handling and user feedback.
There are a couple of missing commas in the error handling section that should be added for consistency with the codebase style:
this.dbtTerminal.error( "applyDeferConfig", "Failed to apply defer configuration", error + ); window.showErrorMessage( `Failed to apply defer configuration: ${error}` + );🧰 Tools
🪛 ESLint
[error] 811-811: Insert
,(prettier/prettier)
[error] 814-814: Insert
,(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/commands/index.ts (1)
src/dbt_client/dbtTerminal.ts (1) (1)
error(105-125)
🪛 ESLint
src/commands/index.ts
[error] 811-811: Insert ,
(prettier/prettier)
[error] 814-814: Insert ,
(prettier/prettier)
grindheim
changed the title
Overview
Problem
To simplify the user experience, we have set up a codespace configuration where the
dbt.deferConfigPerProjectis preconfigured, and we have a task that automatically downloads the manifest from project.However, the user still needs manually go to Actions and toggle the "Enable defer to production" off and on again in order to activate the deferral with the downloaded manifest.
We'd like to be able set up the environment including deferral end to end without any user actions, and in order to do this, the dbt Power User extension needs to provide a command for applying defer config, much like what happens when toggling the button, so that this task can be triggered by us as soon as the updated manifest has been downloaded.
E.g.
Solution
The suggested solution adds a new command to
package.json, and also registers the command insrc\commands\index.ts.Current issues
As there were multiple
applyDeferConfigfunctions configured in the project, I was unsure which one to use. I settled on one defined in DBTProject. However, the issue is here how to trigger this function.The current implementation seems to not be able to fetch the current project, and can thus not run
applyDeferConfig.This seems to be the error message behind the scenes:
NameError: name 'project' is not defined.I'd appreciate some input on which changes I should implement to make it work as it should
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Add
dbtPowerUser.applyDeferConfigcommand to apply defer configuration automatically, but it currently fails due to project fetching issues.dbtPowerUser.applyDeferConfigcommand inpackage.jsonto apply defer configuration automatically.src/commands/index.tsto apply defer configuration to all projects usingproject.applyDeferConfig().applyDeferConfigto fail withNameError: name 'project' is not defined.This description was created by
for cdb82e1. It will automatically update as commits are pushed.
Summary by CodeRabbit