search

calvinmetcalf / crypto-pouch

plugin for encrypted pouchdb/couchdb databases
MIT License
243 stars 43 forks source link

feature/support-salt-in-options #80

Open tahpot opened 3 years ago

garbados commented 3 years ago

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

tahpot commented 3 years ago

@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.

garbados commented 3 years ago

@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 commented 3 years ago

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.

tahpot commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

garbados commented 3 years ago

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

garbados commented 3 years ago

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.