-
Notifications
You must be signed in to change notification settings - Fork 87
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 setting of Master Password via Environment Variable #57
allow setting of Master Password via Environment Variable #57
Conversation
…ng one additional file storage concern, tests passed
@@ -82,6 +83,22 @@ func TestGetPass(t *testing.T) { | |||
if (strings.Compare(pass1Example1, pass1Example1Retry) != 0) || (strings.Compare(pass1Example1Seed1, pass1Example1Seed1Retry) != 0) { | |||
t.Fatal("passwords with same invocation options do not match") | |||
} | |||
|
|||
// Testing GOKEY_MASTER environment variable | |||
os.Setenv("GOKEY_MASTER", "pass1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is useless, because we're not exercising the code from the cmdline utility at all. This test just goes through the library functions, but env handling is done via cmdline tool.
I think we need a different test here, which invokes gokey
tool twice:
- once specifying the password on the cmdline and
- once specifying the password in env variable
and compare the results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a pattern to run sub-process commands within a testing scope or run I suppose that would be one option. I am not aware of others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably add commands directly to https://github.com/cloudflare/gokey/blob/main/.github/workflows/ci.yml (we would need to compile the cmdline tool first anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still haven't resolved this
can you also, please, rebase your branch as I've fixed the tests for >= Go 1.19 in #59 ? |
…ng one additional file storage concern, tests passed
9954045
to
3b5ee8f
Compare
Rebased. |
Ready for review. Thank you for the suggestions and corrections. |
Corrected the go.mod. |
reverted chars to use upper/lower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a cmdline test testing the feature
@dannyfast Thanks a ton for taking this on, exactly the feature I needed 👍🏽 Is this already functional of your branch? |
Moved and updated the code in #72:
|
Linking issue #56