npryce / adr-tools

Command-line tools for working with Architecture Decision Records
Other
4.63k stars 588 forks source link

Allow specification of adr-dir via env var #48

Open martinklepsch opened 6 years ago

martinklepsch commented 6 years ago

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?

npryce commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

bcolferzd commented 6 years ago

I like it

On Tue, Jun 26, 2018 at 4:06 AM Nat Pryce notifications@github.com wrote:

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 doc/adr exists and $ADR_DIR <> "doc/adr" then fail else use $ADR_DIR end else ... current behaviour end

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/npryce/adr-tools/issues/48#issuecomment-400268959, or mute the thread https://github.com/notifications/unsubscribe-auth/AVfOQE095pINLBhgP8zJNjoxjLNQ44jSks5uAhXBgaJpZM4Sxzlz .

-- Brian Colfer Release Engineer, Zendesk