Skip to content

Add Env File Handling to Makefile Generation#5

Open
pokatomnik wants to merge 1 commit into
RaphSku:mainfrom
pokatomnik:feature/env
Open

Add Env File Handling to Makefile Generation#5
pokatomnik wants to merge 1 commit into
RaphSku:mainfrom
pokatomnik:feature/env

Conversation

@pokatomnik
Copy link
Copy Markdown

Hey!

I've added including env files listed in user's configuration.

Copy link
Copy Markdown
Owner

@RaphSku RaphSku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot for the contribution, a very cool addition.
If you could address the comments and also provide unit tests for the changes, that would be great. I would merge it afterwards :)

Comment thread internal/config/apply.go
}

// -- Env files
cm.logger.Info("Inserting env file includes", zap.Strings("func", cm.Config.EnvFiles))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cm.logger.Info("Inserting env file includes", zap.Strings("func", cm.Config.EnvFiles))
cm.logger.Info("Inserting env file includes", zap.Strings("func", "applyConfig"))

Comment thread internal/config/config.go
}

type Config struct {
EnvFiles []string `yaml:"env"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EnvFiles []string `yaml:"env"`
EnvFiles []string `yaml:"envFiles"`

Comment thread README.md
An example configuration file might look like this:
```yaml
---
env:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread README.md
```

### Env files
List all env files that should be used inside the Makefile
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be cool to improve the description by describing what this field does or what function it has in a Makefile.

var content SuggarString

for _, envFile := range envFiles {
content.appendString("-include").appendString(" ").appendString(envFile).lineBreak()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be cool, if the user could decide whether the include should be optional or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants