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

feature/support-salt-in-options #80

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

tahpot
Copy link

@tahpot tahpot commented Aug 31, 2021

No description provided.

@tahpot tahpot marked this pull request as ready for review August 31, 2021 12:43
@tahpot tahpot changed the title feature/support-key-import feature/support-salt-in-options Aug 31, 2021
@garbados
Copy link
Collaborator

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

@tahpot
Copy link
Author

tahpot commented Aug 31, 2021

@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
Copy link
Collaborator

garbados commented Sep 1, 2021

@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
Copy link
Collaborator

garbados commented Sep 1, 2021

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
Copy link
Author

tahpot commented Sep 2, 2021

@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
Copy link
Author

tahpot commented Sep 2, 2021

@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
Copy link
Author

tahpot commented Sep 17, 2021

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

@garbados
Copy link
Collaborator

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

@garbados
Copy link
Collaborator

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.

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.

2 participants