-
Notifications
You must be signed in to change notification settings - Fork 65
Fix credential item category and add another credential fields #210
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
base: main
Are you sure you want to change the base?
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
- Introduced TestAccItemDataSourceApiCredential to verify the retrieval of API credential as an item from the data source. - Added utility functions for generating test API credential items and fields.
- Correctly assigns credential field values during item data source processing. - Removes redundant logic that incorrectly parsed "credential" fields.
Add support for `valid_from`, `expires`, and `filename` fields in the `API_CREDENTIAL` category. Update documentation and internal descriptions to reflect these additions, ensuring consistent handling of API credential metadata.
|
Hi @speto 👋 I just tried to reproduce this issue and was able to access the credential field of an API Credential. It looks like this was resolved in version 2.1.0. I am going to close this PR but if you continue to encounter any more issues please tag me with some context and I can reopen! |
|
Thanks @JillRegan. While the credential field is accessible now, I believe the current implementation is incomplete and inconsistent with the codebase. The condition
Eventually, this should be addressed to align the parsing logic and include the missing fields. |
|
Thank you for your feedback, @speto. I appreciate you pointing out these issues. I'm going to reopen this PR for review. We've added E2E tests since the PR was originally opened so we will want to look at adding testing there for these updates. We're planning to address some open issues in the Terraform Provider over the coming weeks, and I'll try to address these updates/concerns as part of that work. I can take a closer look at these changes once we tackle some existing bugs. 👍 |
I believe part of the problem with the credential category lies in
if f.ID == "credential" && item.Category == "API_CREDENTIAL"where"credential"is checked againstf.ID.I'm not sure if I'm following correctly everything, but I believe the correct
ffield for this comparison should beLabelnotID. Leaving it like rest of the fields on 350-362 should be sufficient.I followed discussion in the original PR but I don't feel much clearer on it. Just think this shouldn't have been merged without tests.
So, first I added a test for credential category, which successfully failed. Then I tried fixing the issue and discovered the
f.IDproblem. Afterwards I added additional credential fields based on what 1password returns for that category and updated the docs. Thetypefield is shared between both database and credential category.This is also related to #52