fix: set credential as password

[SMillerDev]

A very naive fix for #52

[asininemonkey]

Could either @volodymyrZotov or @jpcoenen please review/merge this? This is a major hindrance whilst item type parity is lacking.

[volodymyrZotov]

@SMillerDev I looked at it; unfortunately, this implementation doesn't work. The field.Purpose value is always empty for api_credential fields.

You can use field.ID to check that there is a credential field:

if field.ID == "credential" {
     data.Set("password", field.Value)
} 

However, that would fix the problem only partially.
The thing is that the ID value for credential field is mutable, purpose is always empty and there is no other field that we can rely on to make sure the right field is found. If you ever change the ID (for example using 1Passsword Connect), the provider won't be able to find it. We need to make some improvements on our side to fix that.

To summarise, let's check field.ID == "credential" for now to make it work and I'll raise that within the team so we decide what we'll do on our end.

[SMillerDev]

Fixed, let me know what else

[volodymyrZotov]

@SMillerDev The change is inside switch statement, which checks f.Purpose. The f.Purpose for the credentials field will always be an empty string. So, it'll never reach if f.ID == "credential" { statement.

Let's put if f.ID == "credential" { after the switch.

[SMillerDev]

Sorry, misunderstood your comment. Does this work?

[volodymyrZotov]

@SMillerDev Thinking more about this, there are several questions I have.

1. Why set the credential into password schema field? Why not create a new credentials property in the schema? Similar as done here

"credential": {
    Description:  "Some description",
    Type:         schema.TypeString,
    Optional:     true,
    Computed:     true,
    Sensitive:    true,
},

So then in to code toy would set it like data.Set("credential", f.Value)

2. I'd extend the if statement a bit more, to make sure we are looking for credential field only if item category is api_credential, aka

if f.ID == "credential" && item.Category == "API_CREDENTIAL" {
    data.Set("credential", f.Value)
}

[SMillerDev]

Those all sound fine. I didn't really want to get too invasive with my changes so I kept them small, but happy to tweak them like that

[volodymyrZotov]

I'd appreciate it if you could do that.

I have another question though. The changes are only for data-source. Is it possible to do the same for the resource types?
I'd like to include that also to keep the consistency between data-source and resources. But that might be something that can be addressed in a separate PR.

[SMillerDev]

If this one works for you I'll add it to the resource too.

[volodymyrZotov]

@SMillerDev, 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! 😊

[SMillerDev]

No problem, it's really not that much time I've spend on this. I'll check again later

[SMillerDev]

Okay, updated with the latest changes. Let me know if this is good and I'll add it to the resource.

[volodymyrZotov]

Hi @SMillerDev . Had a chance to test it. It works fine.👍
I see that ci job for documentation doesn't pass. You can fix it by running the following command.

go run github.com/hashicorp/terraform-plugin-docs/cmd/tfplugindocs generate -provider-name onepassword

Then just push updated documentation.

[SMillerDev]

Cool, I think I can do that and the resource today.

[volodymyrZotov]

@SMillerDev I've just noticed that the test ci job is failing. It looks like that happens after you add the credential field to the data source. I didn't have a chance to take a look at what should be the fix for that, but let me know if you need help to figure this out.

[SMillerDev]

I think it would certainly make it faster if you can give some instructions, I don't have that much time (or interest) in a deep dive into tf provider testing.

You're also very welcome to push to my branch or just copy my changes if that's easier. I'm not doing this for my portfolio/credits, I'd just like to use credentials in the data source 😁

[volodymyrZotov]

✅ Approved it.

You can fetch 'credential' items using data-source now. The resource is not supported in this PR and can be added in a separate one.