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 .dazelrc.local support #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

markkohdev
Copy link

This PR resolves #47

It adds the ability to provide dazel parameter overrides via a file called .dazelrc.local. This is useful for things like mounting user-specific paths to dazel and overriding config values for some local enviroments (perhaps if some developers prefer different ports to be exposed than others).

This .dazelrc.local file should be gitignored as mentioned in the readme since the purpose of it is to contain different values for different developers.

Copy link
Collaborator

@mishas mishas left a comment

Choose a reason for hiding this comment

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

Hello Mark,

Thank you so much for your contribution!
I've left you 2 comments, but otherwise this is great work!

Thanks again!

* A .dazelrc file in the current directory.
You can configure dazel in three ways (listed in order of lowest precedence to highest):
* A `.dazelrc` file in the current directory.
* A `.dazelrc.local` file in the current directory (this is for local user overrides - _should be .gitignored if used_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make more sense to have the "local" file in the homedir (so that it'll be loaded from ~/.dazelrc.
It's the more standard way of doing that.

Copy link
Author

Choose a reason for hiding this comment

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

I had considered that but at least for our use case it seems that these local overrides may/will change between projects which use dazel. I can change this if you feel that the local file in the $HOME dir makes the most sense, but perhaps it makes sense to have a few different levels of options.
Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I'm working on implementing this when I have a few minutes. Standby for updates

dazel.py Outdated
@@ -100,6 +101,7 @@ def __init__(self, instance_name, image_name, run_command, docker_command, docke
@classmethod
def from_config(cls):
config = cls._config_from_file()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please pass the filename/env_var params here as well, and remove default values from the function. It will make things more explicit.

Copy link
Author

Choose a reason for hiding this comment

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

👍

@markkohdev
Copy link
Author

@mishas made those changes. Let me know what you think.

@markkohdev
Copy link
Author

Bump @mishas

# Build the expanded path for the config file and join it to the dazel_dir if
# necessary
expanded_path = os.path.expanduser(filename)
if expanded_path.startswith('/'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Use consistent quotes (use double quotes here)

@@ -99,7 +105,9 @@ def __init__(self, instance_name, image_name, run_command, docker_command, docke

@classmethod
def from_config(cls):
config = cls._config_from_file()
config = {}
for (env_var, default_path) in DAZEL_RC_FILE_ENVS:
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for parenthesis

@mishas
Copy link
Collaborator

mishas commented May 16, 2019

Hello @markkohdev again,

Sorry for the long wait on that.
Since I didn't have time to review or think about it, I asked @KimiNewt to take a look.
I can see he left some comments, please fix those, and I'll merge once those are fixed.

Thank you very much for your contribution, and again, sorry for the delay.

@psigen
Copy link

psigen commented Jul 31, 2019

It seems like this is a really small set of fixes, but I'm not sure if @markkohdev is still active.

Is there some way for someone else to just push the changes? Can I make the changes in a new PR based on this one?

@mishas
Copy link
Collaborator

mishas commented Jul 31, 2019

@psigen , works for me, thanks for taking this on.

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.

Allow for additional "local .dazelrc"
4 participants