wireguard-vanity-keygen icon indicating copy to clipboard operation
wireguard-vanity-keygen copied to clipboard

Modulize the tool

Open shmul opened this issue 3 years ago • 2 comments

Hi,

I really like this tool and decided to embed it into an internal tool I'm building. As you provided only a cli option, I took the liberty of restructuring the code into a reusable module and a cli tool wg-vanity-keygen. I hope you'll find this change useful and merge it into the main codeline.

Cheers, Shmul

shmul avatar Feb 18 '22 15:02 shmul

Hi @shmul. Firstly thank you for your contribution, I had never considered the idea of someone using it as a library, so I think this is a great idea (I'd be really interested to know what you are using it for)!

There are a few issues & questions I have with your MR:

  1. The CLI does not return any results when used without the -s flag (ie runs, but doesn't print any private/public keys).
  2. The CLI -s flag is supposed to "print results when all are found", however your code/logic is reversed (-s returns results as they are found).
  3. What exactly is the purpose of the CLI -s flag? I am just trying to justify when someone would actually use that.
  4. There is no documentation in the README.md as to how to use the library.
  5. The New() function and the new structs do not have any leading code comments, so it is unclear how they relate to each other and what they are.

Are you able to please look into those for me and make the necessary changes/fixes etc in your branch? Thanks!

axllent avatar Feb 18 '22 23:02 axllent

Hi,

I am developing an internal tool that provisions and monitors WireGuard peers and the use of vanity keys simplifies the usage and management of keys. I thought it would be easier for my users to have a single tool and therefore I prefer to integrate your work than to run an additional standalone cli tool. I hope it makes sense. I just push an update with a fix to the use of the summary flag. Essentially I think it is needed not so much for the cli use case, but for the use case of programmatic use (like what I intend to do), where it is easier to process the results in batch. I hope it makes sense. I will add comments and update the README file as you requested. I also believe the implementation I provided doesn't yet elegantly addresses the creation of the WordMap as it assumes the caller (main.go for now) knows how to create it. I will handle it as well.

Cheers, Shmul

shmul avatar Feb 19 '22 14:02 shmul

Sorry about the delay - we got hit hard last week with a cyclone. I have rearranged the layout somewhat as this is primarily a stand-alone application, so rather than put the main in a subfolder, I have put the keygen code in a subfolder. Before I merge anything in, could you please have a look at the feature/modulize branch and let me know if this would work for your purpose?

axllent avatar Feb 24 '23 11:02 axllent

Oh wow, I hope things are back to normal now. Yep, this new code layout seems great. I'll track the repo and switch to it when it is merged to master.

Cheers, Shmul

On Fri, Feb 24, 2023 at 1:48 PM Ralph Slooten @.***> wrote:

Sorry about the delay - we got hit hard last week with a cyclone. I have rearranged the layout somewhat as this is primarily a stand-alone application, so rather than put the main in a subfolder, I have put the keygen code in a subfolder. Before I merge anything in, could you please have a look at the feature/modulize https://github.com/axllent/wireguard-vanity-keygen/tree/feature/modulize branch and let me know if this would work for your purpose?

— Reply to this email directly, view it on GitHub https://github.com/axllent/wireguard-vanity-keygen/pull/2#issuecomment-1443573643, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFLE3FTLCWJ3XYQDMJRRVTWZCNYTANCNFSM5OYKIAKA . You are receiving this because you were mentioned.Message ID: @.***>

shmul avatar Mar 10 '23 13:03 shmul

Things here are back to normal, but the entire region I live in is devastated unfortunately. Thanks again for your help & PR, I have merged and released the changes in 0.0.5 :+1:

axllent avatar Mar 12 '23 02:03 axllent