crypto-pouch icon indicating copy to clipboard operation
crypto-pouch copied to clipboard

Published 20 hours ago • verified publisher icon calvinmetcalf

feature/support-salt-in-options

Open tahpot opened this issue 3 years ago • 9 comments

tahpot avatar Aug 31 '21 12:08 tahpot

Hey, thanks for submitting this PR. But, why did you delete the CI file?

garbados avatar Aug 31 '21 20:08 garbados

@garbados

Ah. Github wouldn't let me push with the CI File there as I don't have permissions to run the CI. It was a bit weird, but you can add it back.

tahpot avatar Aug 31 '21 22:08 tahpot

@tahpot Can you share the error the GitHub gave you? That behavior, of rejecting PRs like this, is deeply concerning. Imagine if every contributor had to delete the workflow just to submit a patch, only for maintainers to have to re-add it!

garbados avatar Sep 01 '21 17:09 garbados

Additionally: This PR will require a test in order to be accepted, and the readme will need to be updated here to explain the new option.

garbados avatar Sep 01 '21 17:09 garbados

@tahpot Can you share the error the GitHub gave you? That behavior, of rejecting PRs like this, is deeply concerning. Imagine if every contributor had to delete the workflow just to submit a patch, only for maintainers to have to re-add it!

TBH, I didn't look too closely at the time. I've just tried re-adding the file and received the following:

To https://github.com/tahpot/crypto-pouch.git
 ! [remote rejected] feature/support-key-import -> feature/support-key-import (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/ci.yaml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/tahpot/crypto-pouch.git'

As I hadn't configured Github to use a PAT on my local repo, I couldn't push this file. I guess the solution here is for every maintainer to configure a PAT with worfklow scope locally before they can commit.

I've done this now and added back the workflow.

tahpot avatar Sep 02 '21 07:09 tahpot

@garbados Apologies for the originally rushed PR.

Readme and tests added.

PS: A huge thank you for your work in updating this package to use tweetnacl and taking over maintenance, it's a key component of what we're building at Verida.

tahpot avatar Sep 02 '21 09:09 tahpot

@garbados Is there anything else required to get this PR merged?

tahpot avatar Sep 17 '21 07:09 tahpot

Hey, sorry for the delay, but it looks like this PR wipes out the trySetup logic. Why?

garbados avatar Oct 13 '21 05:10 garbados

It looks like you're using this branch for something else right now, per using a fork of the underlying Crypt library: https://github.com/calvinmetcalf/crypto-pouch/pull/80/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R28

It's hard for me to review this positively in this state.

garbados avatar Oct 13 '21 05:10 garbados