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

Introduce CLI (#14) #70

Merged
merged 11 commits into from Oct 7, 2023
Merged

Introduce CLI (#14) #70

merged 11 commits into from Oct 7, 2023

Conversation

blackheaven
Copy link
Contributor

No description provided.

@cdepillabout
Copy link
Owner

This seems like a reasonable CLI tool to provide.

I think the README, CHANGELOG, etc, still need to be cleaned up, but the general direction seem reasonable.

@Vlix any objection to providing a tool like this?

Copy link
Owner

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

Left a few comments if we decide we want this CLI tool.

password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/ChangeLog.md Outdated Show resolved Hide resolved
password-cli/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Vlix Vlix left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've left a few comments with some questions and/or suggestions, but at first glance it seems fine as a first MVP. (If we want to provide a useful, secure CLI, this will definitely take many many additions and tweaks in the coming years 😅 )

password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/password-cli.cabal Outdated Show resolved Hide resolved
@Vlix
Copy link
Collaborator

Vlix commented Oct 1, 2023

@Vlix any objection to providing a tool like this?

This is actually one of the issues that we have open right now #14 😄

@blackheaven Thank you for the effort of setting this up 👍
It'll probably take a good few iterations before we're happy with a first release of a password CLI, but we have to start somewhere. 😎

@Vlix
Copy link
Collaborator

Vlix commented Oct 1, 2023

I'd also like to see some tests for the CLI to make sure it functions as expected.
Not sure how those would look like. Maybe something using the process library?

@blackheaven
Copy link
Contributor Author

updated, let me know what you think

@cdepillabout
Copy link
Owner

I'd also like to see some tests for the CLI to make sure it functions as expected.
Not sure how those would look like. Maybe something using the process library?

One of the problems with this is that Cabal / cabal-install / stack don't really give you any guarantee what sort of environment test-stanza's will be run in. So you're not really guaranteed that the test suite will be run in an environment with the executable available.

I don't necessarily think we shouldn't add tests like this, but let's make it either disabled by default, or add a flag to the .cabal file to easily disable it for users that have trouble with it because of their build system.

password-cli/app/Main.hs Outdated Show resolved Hide resolved
@Vlix
Copy link
Collaborator

Vlix commented Oct 2, 2023

I've tested a bit, and would indeed like to have the hSetEcho stdin (that's the handle to turn echoing off when typing in the terminal, right?) set to False during requesting the password. (set it back to True with finally)
Also, doing password-cli hash {algo} --help doesn't actually give any explanation of the options. Only a summary of them at the top in the USAGE section.

And a few more requests:

  • Add - password-cli to the stack.yaml's packages: array
  • Bump the resolver to 21.13

password-cli/app/Main.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Vlix Vlix left a comment

Choose a reason for hiding this comment

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

It's beginning to look pretty good.

Thanks for all the effort 😄

password/ChangeLog.md Outdated Show resolved Hide resolved
password-cli/password-cli.cabal Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
password-cli/app/Main.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Vlix Vlix left a comment

Choose a reason for hiding this comment

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

@cdepillabout Do you want to take one more look and check if anything essential is missing? Or anything unsafe / unsecure?

I'm probably going to add more documentation and extend the README after this is merged in, and maybe add some extra options while I'm at it.
Before publishing, we probably want to make the CLI a bit more feature complete, but this is a solid start, methinks, and this PR can at least be merged into the main code base.

password/ChangeLog.md Show resolved Hide resolved
Copy link
Owner

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

@Vlix Vlix merged commit 23a2098 into cdepillabout:master Oct 7, 2023
18 checks passed
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.

None yet

3 participants