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

Readd KeystoreExt to LocalKeystore #585

Merged
merged 5 commits into from
Jun 16, 2023
Merged

Readd KeystoreExt to LocalKeystore #585

merged 5 commits into from
Jun 16, 2023

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Jun 13, 2023

Readded KeystoreExt but in a separate file this time, so it does not get deleted accidentally.

closes #584

@haerdib haerdib self-assigned this Jun 13, 2023
@haerdib haerdib changed the title readd KeystoreExt Readd KeystoreExt to LocalKeystore Jun 13, 2023
@haerdib haerdib requested a review from masapr June 13, 2023 16:41
@haerdib haerdib marked this pull request as ready for review June 13, 2023 16:49
Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

I'm ok with moving the KeystoreExt to a new file. But to prevent the KeystoreExt to be deleted again, we should rather add a test, that uses the KeystoreExt.

@haerdib
Copy link
Contributor Author

haerdib commented Jun 14, 2023

Good input. I will implement a Test doing just that

@haerdib haerdib marked this pull request as draft June 14, 2023 11:35
@haerdib haerdib marked this pull request as ready for review June 15, 2023 16:01
@haerdib haerdib requested a review from masapr June 15, 2023 16:01
Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

I looked into testing without using the filesystem. I ended up, writing a simple test for 'generate()', I think, it's well enough tested now. I'm not sure, if it's possible to properly test the 'public_keys()' with an in-memory store...

If you're ok with my change, then you can merge it.

@haerdib haerdib merged commit 319c94a into master Jun 16, 2023
@haerdib haerdib deleted the bh/read-keystore-ext branch June 16, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readd KeystoreExt
2 participants