-
Notifications
You must be signed in to change notification settings - Fork 561
support digger planfile #1977
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
support digger planfile #1977
Conversation
Warning Rate limit exceeded@motatoes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe update modifies the Changes
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
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.
PR Summary
Adds support for exposing Terraform plan file paths via DIGGER_PLANFILE
environment variable in the execution library, enabling third-party tools to analyze plan outputs.
- Modified
libs/execution/execution.go
to inject local plan file path intoRunEnvVars
during plan stage for custom command steps - Enhancement facilitates integration with external tools like security scanners and cost estimators that need plan file access
1 file reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/execution/execution.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
// setting additional env vars for run step | ||
if d.RunEnvVars == nil { | ||
d.RunEnvVars = make(map[string]string) | ||
} | ||
slog.Debug("adding plan file path to environment", "DIGGER_PLANFILE", d.PlanPathProvider.LocalPlanFilePath()) | ||
d.RunEnvVars["DIGGER_PLANFILE"] = d.PlanPathProvider.LocalPlanFilePath() | ||
_, _, err := d.CommandRunner.Run(d.ProjectPath, step.Shell, commands, d.RunEnvVars) |
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.
🛠️ Refactor suggestion
Propagate DIGGER_PLANFILE
to "apply"
run steps as well
Great to see the map‐initialisation guard and the debug log.
However, Apply()
contains an almost identical "run"
block (lines 370-379) that still receives d.RunEnvVars
without the injected variable, so any helper scripts executed after terraform apply
will not see DIGGER_PLANFILE
.
Consider extracting the injection into a small helper and re-using it in both places:
+func (d *DiggerExecutor) injectPlanfileEnv() {
+ if d.RunEnvVars == nil {
+ d.RunEnvVars = make(map[string]string)
+ }
+ if _, exists := d.RunEnvVars["DIGGER_PLANFILE"]; !exists {
+ slog.Debug("adding plan file path to environment", "DIGGER_PLANFILE", d.PlanPathProvider.LocalPlanFilePath())
+ d.RunEnvVars["DIGGER_PLANFILE"] = d.PlanPathProvider.LocalPlanFilePath()
+ }
+}
and then
-// setting additional env vars for run step
-if d.RunEnvVars == nil {
- d.RunEnvVars = make(map[string]string)
-}
-slog.Debug("adding plan file path to environment", "DIGGER_PLANFILE", d.PlanPathProvider.LocalPlanFilePath())
-d.RunEnvVars["DIGGER_PLANFILE"] = d.PlanPathProvider.LocalPlanFilePath()
+// inject plan file env var
+d.injectPlanfileEnv()
Repeat the same call in Apply()
before the CommandRunner.Run
.
This keeps behaviour consistent and avoids code duplication.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// setting additional env vars for run step | |
if d.RunEnvVars == nil { | |
d.RunEnvVars = make(map[string]string) | |
} | |
slog.Debug("adding plan file path to environment", "DIGGER_PLANFILE", d.PlanPathProvider.LocalPlanFilePath()) | |
d.RunEnvVars["DIGGER_PLANFILE"] = d.PlanPathProvider.LocalPlanFilePath() | |
_, _, err := d.CommandRunner.Run(d.ProjectPath, step.Shell, commands, d.RunEnvVars) | |
// ----- Add this helper somewhere (e.g. right after the DiggerExecutor type) ----- | |
func (d *DiggerExecutor) injectPlanfileEnv() { | |
if d.RunEnvVars == nil { | |
d.RunEnvVars = make(map[string]string) | |
} | |
if _, exists := d.RunEnvVars["DIGGER_PLANFILE"]; !exists { | |
slog.Debug("adding plan file path to environment", "DIGGER_PLANFILE", d.PlanPathProvider.LocalPlanFilePath()) | |
d.RunEnvVars["DIGGER_PLANFILE"] = d.PlanPathProvider.LocalPlanFilePath() | |
} | |
} | |
// ----- In the Run() method (around lines 288–294), replace the inline injection block with ----- | |
// inject plan file env var | |
d.injectPlanfileEnv() | |
_, _, err := d.CommandRunner.Run(d.ProjectPath, step.Shell, commands, d.RunEnvVars) |
🤖 Prompt for AI Agents
In libs/execution/execution.go around lines 288 to 294, the environment variable
DIGGER_PLANFILE is injected into d.RunEnvVars only in the run step, but the
apply step (lines 370-379) lacks this injection, causing inconsistency. To fix
this, extract the DIGGER_PLANFILE injection logic into a helper function that
initializes d.RunEnvVars if nil and sets the variable, then call this helper
both in the run step and before the CommandRunner.Run call in the apply step to
ensure consistent environment setup and avoid code duplication.
Summary by CodeRabbit