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

Allow specification of adr-dir via env var #48

Open
martinklepsch opened this issue Mar 20, 2018 · 8 comments
Open

Allow specification of adr-dir via env var #48

martinklepsch opened this issue Mar 20, 2018 · 8 comments

Comments

@martinklepsch
Copy link
Contributor

Having a file like .adr-dir at the root of a repo is somewhat of a thorn in the eye of people trying to keep the root of their repo as clean/approachable as possible.

Instead of configuring adr-tools using files it would be nice if we could use an env var to specify the ADR dir. By allowing that users could easily wrap adr itself in scripts which configure it in whatever way desired.

Would you be open for a PR that enables setting the ADR dir via an env var?

  • Name suggestion: $ADR_DIR
  • IMHO it should have precedence over the .adr-dir file but not super important to our specific use.
@npryce
Copy link
Owner

npryce commented Jun 4, 2018

Sounds like a good idea. Not sure about precedence. I think for safety, it should be an error (or at least a highly visible warning) if the environment variable is set and the .adr-dir file exists.

This would also address the issues raised in #51.

@npryce
Copy link
Owner

npryce commented Jun 25, 2018

Thinking about this some more. Yes, an ADR_DIR environment variable sounds like a good idea (although I worry about environment variable name clash... is the name used by any other program?)

The advantage of the .adr-dir file over the environment variable is that it can be checked into version control, while the environment variable is local to each user's workstation

Precedence-wise, therefore, the .adr-dir file MUST take precedence over the environment variable, otherwise setting the environment variable would stop adr-tools working with existing projects. For the same reason, if the default ADR directory (doc/adr) already exists, that also must take precedence over the environment.

@retnuh
Copy link

retnuh commented Jun 25, 2018

Isn't this a bit backwards with expectations though? One often uses an ENV var to override what's checked in of the default state; it allows easy switching of behaviour at the cli without having to edit files, move directories, etc. I think it would make sense to issue either a warning or error if there is a conflict but I would think that the main utility would be having the ADR_DIR be the highest precedence.

@npryce
Copy link
Owner

npryce commented Jun 25, 2018

I'm torn... I can see one way being error prone. And the other way being unusual (and therefore error prone in different ways).

A design goal is that all project config can be checked into version control. Maybe the conflict here is because an environment variable works against that goal.

@retnuh
Copy link

retnuh commented Jun 25, 2018

Looking back at original use case reported in the issue, I think your first response makes the most sense - warn/puke if env var exists as well as .adr-dir and they don't match. Could also have a flag to suppress the warning/error.

If I had to guess, people will use it in an either-or sort of way - either have a checked in .adr-dir file, or use ENV vars to switch/specify based on their own custom setups. The appropriate documentation could even be written to suggest that users pick one way or the other, perhaps?

@npryce
Copy link
Owner

npryce commented Jun 26, 2018

I think the rules should be the following pseudocode:

if ADR_DIR is set
    if .adr-exists and the contents is different than $ADR_DIR or the directory doc/adr exists and $ADR_DIR != "doc/adr" then
        fail
    else
        use $ADR_DIR
    end
else
    ... current behaviour
end

@npryce
Copy link
Owner

npryce commented Jun 26, 2018

This can easily be implemented by adding logic to the _adr_dir script.

@bcolferzd
Copy link

bcolferzd commented Jun 26, 2018 via email

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

No branches or pull requests

4 participants