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

implement dedicated summon provider (#1436) #1438

Merged
merged 4 commits into from Jul 14, 2020

Conversation

ckolumbus
Copy link
Contributor

@ckolumbus ckolumbus commented Jun 28, 2020

initial implementation of a dedicated summon-provider implementation for gopass as the default output behavior of gopass violates the summon contract by adding all meta data.
This implementation outputs only the secret, no other special handling is implemented.

Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

This looks very good already.
Some cleanup and finishing the tests and this should be ready to merge.

cmd/gopass-summon-provider/summon-provider.go Outdated Show resolved Hide resolved
cmd/gopass-summon-provider/summon-provider_test.go Outdated Show resolved Hide resolved
Signed-off-by: Chris Drexler <ckolumbus@ac-drexler.de>
Signed-off-by: Chris Drexler <ckolumbus@ac-drexler.de>
Signed-off-by: Chris Drexler <ckolumbus@ac-drexler.de>
Signed-off-by: Chris Drexler <ckolumbus@ac-drexler.de>
@ckolumbus ckolumbus force-pushed the fix/summon-provider-support branch from f341f08 to 770739f Compare July 6, 2020 07:38
@ckolumbus
Copy link
Contributor Author

Test implemented, refactoring of outputting done. How can i fix the CommitCheck errors?

@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2020

Codecov Report

Merging #1438 into master will decrease coverage by 0.24%.
The diff coverage is 17.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
- Coverage   64.34%   64.09%   -0.24%     
==========================================
  Files         203      205       +2     
  Lines       11592    11647      +55     
==========================================
+ Hits         7458     7465       +7     
- Misses       3323     3370      +47     
- Partials      811      812       +1     
Impacted Files Coverage Δ
cmd/gopass-summon-provider/main.go 0.00% <0.00%> (ø)
cmd/gopass-summon-provider/summon-provider.go 77.78% <77.78%> (ø)
pkg/ctxutil/ctxutil.go 90.50% <0.00%> (-4.31%) ⬇️
internal/action/show.go 46.09% <0.00%> (-1.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9f3834...770739f. Read the comment docs.

@dominikschulz
Copy link
Member

CommitCheck wants a RELEASE_NOTES=... tag in all commits. This will be used to assemble the release notes.

@ckolumbus
Copy link
Contributor Author

CommitCheck wants a RELEASE_NOTES=... tag in all commits. This will be used to assemble the release notes.

@dominikschulz could you maybe provide a link to some examples? What are the best practices for this in case of several (unimportant) check-ins? Squashing?

@dominikschulz
Copy link
Member

Yes, we usually squash commits in a PR.

@ckolumbus
Copy link
Contributor Author

@dominikschulz sorry for bothering you again:

CommitCheck wants a RELEASE_NOTES=... tag

so I squash everything and add one RELEASE_NOTES tag?

@dominikschulz
Copy link
Member

Yes, that's how I'd it.
I'd prefer to only require that tag for the merge commit but couldn't figure out how to do that reliably.

@dominikschulz dominikschulz merged commit 31aff1b into gopasspw:master Jul 14, 2020
@ckolumbus ckolumbus deleted the fix/summon-provider-support branch October 28, 2020 15:52
kpitt pushed a commit to kpitt/gopass that referenced this pull request Jul 21, 2022
…upport

implement dedicated summon provider (gopasspw#1436)

RELEASE_NOTES=[FEATURE] Add summon provider
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.

None yet

3 participants