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

Replace ids with checksum of the resource ids #869

Merged
merged 5 commits into from
Nov 20, 2020

Conversation

pecigonzalo
Copy link
Contributor

Fixes: #764

I was not sure if you would prefer to use sha1 or some other function, but I think this should get the job done.

I chose to leave generateShaId as a separate function but repeat sort and .join because I thought there could be non list ids we could want to produce a sha for, but then noticed we do not. I think leaving it reusable its not bad, but let me know if this is not your preferred choice.

@pecigonzalo
Copy link
Contributor Author

I chose to leave generateShaId as a separate function but repeat sort and .join because I thought there could be non list ids we could want to produce a sha for, but then noticed we do not. I think leaving it reusable its not bad, but let me know if this is not your preferred choice.

Decided this made no sense and merged the sort and sha.

@jacobbednarz
Copy link
Member

jacobbednarz commented Nov 12, 2020

Thanks but we already have stringChecksum for this purpose. Are you able to update this PR to use that function instead?

@pecigonzalo
Copy link
Contributor Author

pecigonzalo commented Nov 12, 2020 via email

@pecigonzalo
Copy link
Contributor Author

pecigonzalo commented Nov 13, 2020

@jacobbednarz I reused the checksum function but kept the wrapper function to make the list -> checksum, in order to avoid repeating the join, etc. Would you prefer to remove that as well?

@@ -110,6 +109,7 @@ func dataSourceCloudflareWAFGroupsRead(d *schema.ResourceData, meta interface{})
}

log.Printf("[DEBUG] Reading WAF Groups")
groupIds := make([]string, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I can't see where we are appending to this slice. Am I missing it?

@@ -96,6 +95,7 @@ func dataSourceCloudflareWAFPackagesRead(d *schema.ResourceData, meta interface{
}

log.Printf("[DEBUG] Reading WAF Packages")
packageIds := make([]string, 0)
Copy link
Member

Choose a reason for hiding this comment

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

@@ -119,6 +118,7 @@ func dataSourceCloudflareWAFRulesRead(d *schema.ResourceData, meta interface{})
}

log.Printf("[DEBUG] Reading WAF Rules")
ruleIds := make([]string, 0)
Copy link
Member

Choose a reason for hiding this comment

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

@jacobbednarz
Copy link
Member

I think the dedicated stringListChecksum function is 👌 to avoid repeating it in each call site 👍 I've just got a couple of questions about appending to the slices which I can only seem to find a single place where we actually build up the slice.

@pecigonzalo
Copy link
Contributor Author

@jacobbednarz Good catch, I completely missed it when copy/pasting this from one data source to another. I believe I covered them all now.

@jacobbednarz jacobbednarz changed the title Replace ids with SHA256 of the resouce ids Replace ids with checksum of the resource ids Nov 20, 2020
@jacobbednarz jacobbednarz merged commit 264d0c1 into cloudflare:master Nov 20, 2020
@jacobbednarz
Copy link
Member

Awesome, thanks @pecigonzalo! You rock 🤘

@rcny
Copy link

rcny commented Nov 23, 2020

Can you release a patch version with this please?

@jacobbednarz
Copy link
Member

Probably not right away - we generally release on a monthly basis unless there are pressing fixes included in it. There are a couple of PRs in flight we'll be looking to include in the next release.

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.

TF13: data.cloudflare_zones reports changes on every plan
3 participants