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

feat: Support 1Password Connect as a backend #303

Merged
merged 8 commits into from
Mar 9, 2022

Conversation

lucasmarshall
Copy link
Contributor

Description

Add support for 1Password Connect servers as a backend

Fixes: #235

Checklist

Please make sure that your PR fulfills the following requirements:

  • Reviewed the guidelines for contributing to this repository
  • The commit message follows the Conventional Commits Guidelines.
  • Tests for the changes have been updated
  • Docs have been added / updated
  • Optional. My organization is added to USERS.md.

Type of Change

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe)

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2022

CLA assistant check
All committers have signed the CLA.

@lucasmarshall lucasmarshall changed the title Lucas/onepasswordconnect Support 1Password Connect as a backend Feb 22, 2022
@lucasmarshall lucasmarshall changed the title Support 1Password Connect as a backend feat: Support 1Password Connect as a backend Feb 22, 2022
docs/backends.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022

Codecov Report

Merging #303 (6822eec) into main (c15b014) will decrease coverage by 0.00%.
The diff coverage is 76.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
- Coverage   76.52%   76.51%   -0.01%     
==========================================
  Files          21       22       +1     
  Lines         920      941      +21     
==========================================
+ Hits          704      720      +16     
- Misses        134      137       +3     
- Partials       82       84       +2     
Impacted Files Coverage Δ
pkg/backends/onepasswordconnect.go 68.75% <68.75%> (ø)
pkg/config/config.go 83.57% <100.00%> (+0.60%) ⬆️

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 c15b014...6822eec. Read the comment docs.

Copy link
Member

@werne2j werne2j left a comment

Choose a reason for hiding this comment

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

Looks good, will let @jkayani take a look before merging

go.mod Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
Copy link
Member

@jkayani jkayani left a comment

Choose a reason for hiding this comment

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

Code, tests and docs look great! Thanks for the work @lucasmarshall. I'd like to resolve at least the dependency question since the OP SDK should be marked as a direct dependency (thinking about build issues if dependencies aren't categorized correctly). The other things aren't big deals but I leave it to you to make any changes if you see fit.

Copy link
Member

@jkayani jkayani left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

go.mod Outdated
@@ -52,6 +52,8 @@ require (
sigs.k8s.io/yaml v1.3.0
)

require github.com/1Password/connect-sdk-go v1.2.0
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, one small thing. I would like this to be up in the above require block. Seems a little odd to have another require block with just one package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. go mod tidy put this here. Not sure why.

Copy link
Member

@werne2j werne2j left a comment

Choose a reason for hiding this comment

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

Thanks @lucasmarshall!

@werne2j werne2j merged commit b434368 into argoproj-labs:main Mar 9, 2022
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

5 participants