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

Add redis_cache datasource #3481

Merged
merged 14 commits into from
May 29, 2019
Merged

Conversation

jackofallops
Copy link
Member

@jackofallops jackofallops commented May 21, 2019

Fixes #3432

Steve Jones added 3 commits May 21, 2019 13:47

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the new datasource @sjones-sot.

I've left some comments inline but my main concern is the deviation from the actual resource. Could we just copy the schema there & set the properties in the same way it does?

Thanks!

azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Show resolved Hide resolved
azurerm/data_source_redis_cache.go Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved

Verified

This commit was signed with the committer’s verified signature.
updated how d.Set's collect properties
aligned properties in docs example
updated example resource names
@ghost ghost added size/XL and removed size/L labels May 21, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @sjones-sot

Thanks for pushing those changes - I've taken a look through and left a few more comments but if we can fix those up this otherwise LGTM 👍

Thanks!

azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache_test.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache_test.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache_test.go Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved

Verified

This commit was signed with the committer’s verified signature.
tidied up multiple calls to testLocation()
simplified test steps and removed unnecessary func for resource setup
updated docs to refer to correct resource
@jackofallops
Copy link
Member Author

I think everything's addressed, except the item from @katbyte
Thanks both for the great feedback and assistance.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @sjones-sot

Thanks for pushing those changes - apologies, in my last review I'd missed that the Schema fields weren't Computed (which is readonly vs Optional which is setable) - if we can fix those up (and ensure those fields are documented) this otherwise LGTM 👍

Thanks!

azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
azurerm/data_source_redis_cache.go Outdated Show resolved Hide resolved
website/docs/d/redis_cache.html.markdown Show resolved Hide resolved
Steve Jones added 2 commits May 22, 2019 15:52

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
… and d.Sets
@tombuildsstuff tombuildsstuff added this to the v1.29.0 milestone May 23, 2019
@jackofallops
Copy link
Member Author

Changes being tested currently - so very slow working/testing with redis resources, will tidy and push asap. (Mentioned as I mistakenly pushed some incorrect changes containing Optional items again, sorry)

Steve Jones and others added 3 commits May 23, 2019 13:31

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Removed MaxItems on readonly resource

Co-Authored-By: Tom Harvey <tombuildsstuff@users.noreply.github.com>
@jackofallops
Copy link
Member Author

I have a (possible) bug in this resource/datasource. Accessing the patch_schedule attribute when there is no schedule generates an error:
output.patch_schedule: Resource 'data.azurerm_redis_cache.test' does not have attribute 'patch_schedule' for variable 'data.azurerm_redis_cache.test.patch_schedule'
similarly for the resource. I don't know if this is desired/expected behaviour, or if an empty value should be returned somehow?

@ghost ghost removed the waiting-response label May 24, 2019
@katbyte katbyte modified the milestones: v1.29.0, v1.30.0 May 25, 2019
Steve Jones added 2 commits May 28, 2019 10:57

Verified

This commit was signed with the committer’s verified signature.
outputs to be used for this item without error, even when there are no schedules.

Verified

This commit was signed with the committer’s verified signature.
@jackofallops
Copy link
Member Author

jackofallops commented May 28, 2019

I'm at a loss at why the linter is failing on my last commit(s), would appreciate a hint if anyone can see what I'm missing?
EDIT: Never mind - minor code-blindness.

Verified

This commit was signed with the committer’s verified signature.
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @sjones-sot

Thanks for pushing those changes - this now LGTM 👍

@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review May 29, 2019 13:34

dismissing since changes have been pushed

@jackofallops
Copy link
Member Author

Awesome - thanks for the assistance and reviews :)

@tombuildsstuff
Copy link
Contributor

Tests pass:

Screenshot 2019-05-29 at 14 52 06

@tombuildsstuff tombuildsstuff merged commit 867a15d into hashicorp:master May 29, 2019
tombuildsstuff added a commit that referenced this pull request May 29, 2019

Verified

This commit was signed with the committer’s verified signature.
@ghost
Copy link

ghost commented Jun 20, 2019

This has been released in version 1.30.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
	version = "~> 1.30.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jun 29, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Redis Cache data source
4 participants