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/document on plugin framework #171

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

sdahlbac
Copy link
Contributor

@sdahlbac sdahlbac commented May 17, 2024

This is a re-implementation of #161 due to the upgrading to the new plugin framework.

I have have only tested this with acceptance and cli, as I do not have a connect.

  • Supports multiple files
  • Supports text documents and binary documents (provided one uses the content_base64 version)
    Roundtrip test:

data "onepassword_item" "foo" {
vault = var.demo_vault
uuid = "..."
}
resource "local_file" "truststore" {
filename = "truststore.jks"
content_base64 = data.onepassword_item.foo.file.2.content_base64
}
(where 2 is the third file for that item, added via the UI)

I had to use the op read approach rather than op document get (unless I am missing an undocumented feature) to be able to handle multiple files.

I have not tried to implement a resource, since as far as I understood connect cannot support it. I added a validation that you cannot create a new onepassword_item with category document. But the error message does not say why you cannot do it.

(While it sucks a bit having to reimplement this out of the blue, I must say that the first impressions of the newer framework feels nicer to work with.)

Relates: #51

@sdahlbac sdahlbac force-pushed the feat/document-on-plugin-framework branch from e89fc62 to f33a1e5 Compare May 20, 2024 06:16
@volodymyrZotov volodymyrZotov self-requested a review May 21, 2024 14:39
@edif2008 edif2008 self-requested a review May 21, 2024 14:39
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.

Summary

Thank you so much for your effort in making this contribution. This is much appreciated and it looks pretty solid.

Functional review: ✅

I've validated the following scenarios and can confirm that they work as expected:

  • A Document item with a file attached to it
  • An item of different category (e.g. Login) with multiple files attached to it.

One thing that would be nice to do here is to put the files in their appropriate section, since this is a possibility through the 1Password app. I don't think it's possible via the CLI though.

I also ran the acceptance tests and they all pass.

Code review: ✅

The code is straight forward and easy to follow. I've left comments in how to make it better.

The thing to call out code-wise is related to file attachments being able to belong to sections as well. This is something that might require some changes to ensure we include this scenario as well.

internal/provider/const.go Outdated Show resolved Hide resolved
internal/provider/onepassword_item_resource_test.go Outdated Show resolved Hide resolved
docs/resources/item.md Outdated Show resolved Hide resolved
internal/provider/test_http_server.go Outdated Show resolved Hide resolved
internal/onepassword/cli/op.go Show resolved Hide resolved
internal/provider/onepassword_item_data_source.go Outdated Show resolved Hide resolved
internal/provider/const.go Outdated Show resolved Hide resolved
@sdahlbac sdahlbac force-pushed the feat/document-on-plugin-framework branch from 348568d to de30fa6 Compare June 5, 2024 20:14
@sdahlbac
Copy link
Contributor Author

sdahlbac commented Jun 5, 2024

@edif2008 The review comments should be adressed now

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.

Functional review: ✅

Tested and validated that files from sections are placed in the appropriate sections in the Terraform state.

Code review: ✅

Code looks much cleaner now. Thank you for addressing the feedback.

Other notes

⚠️ It seems that the documentation check fails since the /docs/data-sources/item.md file is not properly updated. Re-running go generate and pushing the changes to that file should fix it.

Once that is addressed, this PR is good to be merged. 😄

@sdahlbac
Copy link
Contributor Author

sdahlbac commented Jun 7, 2024

@edif2008 done

@sdahlbac
Copy link
Contributor Author

sdahlbac commented Jun 7, 2024

@edif2008 fixed

@edif2008 edif2008 merged commit 1e24ccf into 1Password:main Jun 17, 2024
3 checks passed
@sdahlbac sdahlbac deleted the feat/document-on-plugin-framework branch June 17, 2024 16:51
@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.

2 participants