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

Add MUTAGEN_PROJECT_FILE env variable #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmera
Copy link

@jmera jmera commented May 16, 2020

By default mutagen project looks for a config file named 'mutagen.yml'
to load project configuration. This change gives us the ability to
customize that with an environment variable.

pkg/project/paths.go Outdated Show resolved Hide resolved
@jmera jmera force-pushed the master branch 2 times, most recently from 90603bc to bcf65aa Compare May 16, 2020 01:07
By default `mutagen project` looks for a config file named 'mutagen.yml'
to load project configuration. This change gives us the ability to
customize that with an environment variable.

Signed-off-by: Julich Mera <julich.mera@gmail.com>
@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #203 into master will decrease coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
- Coverage   49.82%   49.74%   -0.09%     
==========================================
  Files         234      235       +1     
  Lines       16314    16320       +6     
==========================================
- Hits         8129     8118      -11     
- Misses       7156     7169      +13     
- Partials     1029     1033       +4     
Impacted Files Coverage Δ
pkg/project/paths.go 0.00% <0.00%> (ø)
pkg/agent/valve.go 84.21% <0.00%> (-10.53%) ⬇️
pkg/filesystem/watching/watch_recursive_windows.go 70.96% <0.00%> (-4.04%) ⬇️
...ng/internal/third_party/winfsnotify/winfsnotify.go 63.12% <0.00%> (-1.07%) ⬇️
pkg/synchronization/endpoint/local/endpoint.go 67.94% <0.00%> (-0.70%) ⬇️
...ing/internal/third_party/notify/watcher_inotify.go 62.78% <0.00%> (+1.34%) ⬆️
pkg/identifier/identifier.go 77.77% <0.00%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5742193...89fb771. Read the comment docs.

@jmera
Copy link
Author

jmera commented May 19, 2020

Hi @havoc-io,

First and foremost, thank you for the fantastic work you do maintaining mutagen! Your hard work does not go unnoticed.

Before I continue with this, do you think it is a feature worth pursing? I opened this PR because I found myself wanting to do this in my project.

@jmera
Copy link
Author

jmera commented May 19, 2020

Another note/disclaimer: I am new to golang. This is the first piece of go code I've ever written so please bear with me as I learn it.

@xenoscopic
Copy link
Member

Hey @jmera, thanks for the pull request! I need a little more time to think about this idea, but I wanted to respond at least a little bit with my initial thoughts.

At the moment, I'm working on automated Docker Compose integration for Mutagen. I mention this because:

  1. I may discontinue support for projects (or at least deprecate/feature freeze them), though this wouldn't be any time soon, and
  2. While trying to integrate with Docker Compose (and needing to emulate some of its behavior), I'm finding that having multiple ways to specify configuration files (e.g. via -f/--file, via standard input, via COMPOSE_FILE/COMPOSE_PATH_SEPARATOR (potentially specified in .env), via implicit path searches, etc.) actually makes things difficult to deal with and reason about. I'd be concerned about introducing the same complexity to Mutagen.

On the other hand, I think other tools are more likely to wrap Mutagen projects than to try to emulate their behavior, so it may not matter. I wouldn't mind a few more days to think about it.

Can you tell me a little about your use case?

If the logic is kept simple (i.e. a single file, no YAML merging, no MUTAGEN_PATH SEPARATOR, etc.) then this might be okay.

Copy link
Member

@xenoscopic xenoscopic left a comment

Choose a reason for hiding this comment

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

I've added two quick review comments. I still wouldn't mind a few more days to mull this over before too much more work gets put into it. In any case, the effort and input is much appreciated!

Comment on lines +14 to +20
func ConfigurationFileName() string {
fileName := os.Getenv("MUTAGEN_PROJECT_FILE")
if fileName == "" {
return DefaultConfigurationFileName
}
return fileName
}
Copy link
Member

Choose a reason for hiding this comment

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

This code looks like it needs to be go fmt'd since it's using spaces for indentation. You can format all of your changes with

go fmt ./cmd/mutagen/project/...
go fmt ./pkg/project/...

(assuming you're in the root of the source directory).

This is one of those weird Go things, but basically Go's canonical style is "whatever the gofmt tool decides." For indentation, Go prefers tabs (though it does use spaces for alignment).

@@ -39,7 +39,7 @@ func startMain(command *cobra.Command, arguments []string) error {
// relative paths (including relative synchronization paths and relative
// Unix Domain Socket paths) to be resolved relative to the project
// configuration file.
configurationFileName := project.DefaultConfigurationFileName
configurationFileName := project.ConfigurationFileName()
Copy link
Member

@xenoscopic xenoscopic May 21, 2020

Choose a reason for hiding this comment

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

If the MUTAGEN_PROJECT_FILE environment variable allows a path to be specified (as opposed to just a filename), then I think the same os.Chdir logic used for a project file specified on the command line will be needed to support relative synchronization paths. This applies at least in the start command, but I think in the other commands as well.

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