-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for global (top-level) dotenv files #18
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
Conversation
Reviewer's Guide by SourceryThis pull request adds support for global dotenv files in task parsing. It introduces the ability to load environment variables from specified .env files and applies them globally to all tasks in the Runfile. Sequence diagram for parsing tasks with global dotenv supportsequenceDiagram
participant User
participant TaskParser
participant DotEnvParser
participant Runfile
User->>TaskParser: ParseTask(ctx, rf, task)
TaskParser->>Runfile: Check DotEnv
alt DotEnv is not nil
TaskParser->>DotEnvParser: parseDotEnvFiles(rf.DotEnv...)
alt DotEnv file found
DotEnvParser-->>TaskParser: Return env variables
TaskParser->>TaskParser: Add env variables to globalEnv
else DotEnv file not found
DotEnvParser-->>TaskParser: Return error
TaskParser-->>User: Return error
end
end
TaskParser->>User: Return ParsedTask
Updated class diagram for Runfile and Task parsingclassDiagram
class Runfile {
+[]string DotEnv
+map[string]Task Tasks
}
class Task {
+bool ignoreSystemEnv
+[]any Commands
}
class ParsedTask {
+[]string Shell
+string WorkingDir
+map<string, string> Env
+[]CommandJson Commands
}
class CommandJson {
+string Command
}
class TaskParser {
+ParseTask(Context ctx, Runfile rf, Task task) ParsedTask
}
class DotEnvParser {
+parseDotEnvFiles(string... files) map<string, string>
}
Runfile --> Task
TaskParser --> Runfile
TaskParser --> DotEnvParser
TaskParser --> ParsedTask
ParsedTask --> CommandJson
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @nxtcoder17 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| if rf.DotEnv != nil { | ||
| m, err := parseDotEnvFiles(rf.DotEnv...) | ||
| if err != nil { |
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.
suggestion: Add context to the returned error for better debugging.
When returning the error from parseDotEnvFiles, consider wrapping it with additional context using fmt.Errorf or a custom error type. This will make it easier to trace the source of the error during debugging.
| if err != nil { | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse .env files: %w", err) |
| } | ||
| } | ||
|
|
||
| if rf.DotEnv != nil { |
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.
suggestion (performance): Add a check for empty rf.DotEnv slice to avoid unnecessary function calls.
Consider adding a check for len(rf.DotEnv) > 0 before calling parseDotEnvFiles to avoid unnecessary function calls when the slice is empty.
| if rf.DotEnv != nil { | |
| if rf.DotEnv != nil && len(rf.DotEnv) > 0 { |
Closes #17
Summary by Sourcery
Add support for global dotenv files in task parsing to enable setting environment variables globally from specified dotenv files. Update tests to cover new functionality, ensuring correct behavior when dotenv files are present or missing.
New Features:
Tests: