search

onelogin / terraform-provider-onelogin

GNU General Public License v3.0
27 stars 19 forks source link

Adding onelogin_user datasource #51

Closed ZsoltPath closed 3 years ago

ZsoltPath commented 3 years ago

Changing the onelogin_users data source to return a list of IDs for a query Adding onelogin_user data source to return the details of a single user

ZsoltPath commented 3 years ago

Hi @dcaponi,

Since the SDK is updated, I've added the proper user data sources.

There's one issue though, I've added the data source documentation to #48 but it hasn't appeared on the registry docs https://registry.terraform.io/providers/onelogin/onelogin/latest/docs I guess it's because there wasn't any data source before and it's missing from the pipeline.

Also, it is possible to get away with that security warning in test? I know md5 is weak, but it doesn't matter here.

dcaponi commented 3 years ago

Hey @ZsoltPath Thanks so much for helping out with the data-sources side of this. TBH I'm not actually much of a Terraform user myself so the help & insight is much appreciated.

For posterity, Im gonna say in writing that this security warning suppression is fine in the context that we are using MD5 for hashing but not for anything secure or sensitive.

I will ask however why you went with MD5 and not Base64 encoding or something like that, just so I'm aware of why that choice was made.

I checked up on provider docs publishing and I think we're just naming the directory incorrectly. We're calling it datasources and this documentation implies the directory should be called data-sources. Could you rename the directory as part of this PR and as soon as I merge & cut a release of this we'll see if that was it.

ZsoltPath commented 3 years ago

Hi @dcaponi,

I've renamed the directory. Hope it helps.

Regarding the md5 hash, I needed something to turn a query structure to a simple string ID for the terraform resource. MD5 looked like the easiest solution. I'm not really a Go developer. If you have a more proper idea, I'm happy to change.

dcaponi commented 3 years ago

Hey @ZsoltPath thanks for the insight. I understand the intent and I think its a valid choice (or at least I don't have any reason to believe its an invalid choice 😄 )

Merging now. Thanks again for the help 🎉

Your changes should be available in version v0.1.16 which should be out today.

dcaponi commented 3 years ago

@ZsoltPath looks like directory rename did the trick, thanks! Release is available now.