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: add ssh key data source #158

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Conversation

atammy-narmi
Copy link
Contributor

@atammy-narmi atammy-narmi commented Mar 4, 2024

Relates: #74

The only thing to add was support for replacing spaces with underscores and adding new fields in the data source. Now going forward it should be easier if someone wants to add more fields to the data source, as the spaces will get fixed easily.
The above comment is no longer valid as the data.Set function is no longer available, so it seems like it needs to be done manually for now.
Would be nice to get this in 😸

@atammy-narmi atammy-narmi changed the title feat: add ssh public key data source feat: add ssh key data source Mar 4, 2024
@atammy-narmi
Copy link
Contributor Author

@volodymyrZotov could take a look please

@volodymyrZotov
Copy link
Contributor

@atammy-narmi Sure! Added this to the review list! Thanks for reminding about this!

@volodymyrZotov volodymyrZotov self-requested a review March 22, 2024 15:19
@atammy-narmi
Copy link
Contributor Author

friendly ping

@volodymyrZotov
Copy link
Contributor

volodymyrZotov commented May 17, 2024

@atammy-narmi, sorry for the delay.

I wanted to bring to your attention that we've recently migrated to the new terraform-plugin-framework. With this big change, this PR is no longer applicable to the latest version of the provider.

We really appreciate the time and effort that you have already put into this PR. If you have some additional time to spare and you're still interested in having this change applied, please go ahead and modify this PR to make it compatible with the latest v2 version of this provider, at which point we'd be happy to review it again.

Thank you! 😊

@edif2008
Copy link
Member

Hey @atammy-narmi!

As an addition to what @volodymyrZotov said, I've noticed that your commits are not signed. This repository requires all commits to be signed. If you work on changing this PR to make it compatible with the latest v2 version of the provider, it would be great to have your commits signed as well.

Here is some guidelines in achieving this.

Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing

@atammy-narmi
Copy link
Contributor Author

OK, I'll work on updating this PR.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

@atammy-narmi atammy-narmi force-pushed the data/ssh-pubkey branch 2 times, most recently from aed8946 to 7c226ed Compare June 4, 2024 15:32
Signed-off-by: Abhinav Tamaskar <abhinav@narmi.com>
@atammy-narmi
Copy link
Contributor Author

OK, I've done the required changes.

Copy link
Member

@edif2008 edif2008 left a comment

Choose a reason for hiding this comment

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

Hey @atammy-narmi! 👋🏻

Summary

Thank you so much for your effort in adding this functionality. It is much appreciated.

Functional review: ✅

I've validated that I can get the SSH public and private key, as well as the notes for the SSH item if it exists.

❗ One thing worth noting is that the private key that the Terraform item data source will have is in PKCS#8 format and not in OpenSSH format. If we want to provide the private key in mutliple formats, it may be a bit more challenging.

Code review: ✅

The code looks straight forward and on point. A couple of things would be nice to add on top of what's been done here:

  • A test case in which we get the SSH Key public and private key. This can be dummy data.
  • It would be nice to include the SSH Key as a valid item category for the data source.

@atammy-narmi
Copy link
Contributor Author

I've added a test. Is there anything blocking this from being merged?

edif2008 added 2 commits June 17, 2024 19:16
# Conflicts:
#	internal/provider/const.go
#	internal/provider/onepassword_item_data_source.go
#	internal/provider/onepassword_item_data_source_test.go
#	internal/provider/test_utils.go
@edif2008 edif2008 merged commit a8d1021 into 1Password:main Jun 17, 2024
3 checks passed
@atammy-narmi atammy-narmi deleted the data/ssh-pubkey branch June 17, 2024 17:38
@github-actions github-actions bot mentioned this pull request Jun 17, 2024
2 tasks
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.

3 participants