Throw error when encountering untracked action secrets

[sergiught]

Description

Fixes #52

Although we can already update and create secrets as this was patched before the repo transfer, because we cannot import the values of the secrets we run the risk of erasing the secrets that are found already in the Auth0 Dashboard for the action.

In this PR we are trying to prevent that from happening in case there are secrets that are not managed through terraform by throwing an error while updating.

Checklist

Note: Checklist required to be completed before a PR is considered to be reviewable.

Auth0 Code of Conduct

• I have read and agreed to the terms within the Auth0 Code of Conduct

Auth0 General Contribution Guidelines

• I have read the Auth0 General Contribution Guidelines

Changes include test coverage?

• Yes
• Not needed

Does the description provide the correct amount of context?

• Yes, the description provides enough context for the reviewer to understand what these changes accomplish

Have you updated the documentation?

• Yes, I've updated the appropriate docs
• Not needed

Is this code ready for production?

• Yes, all code changes are intentional and no debugging calls are left over

[codecov-commenter]

Codecov Report

Merging #248 (2a12da9) into main (3e7f2b4) will decrease coverage by 0.05%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main     #248      +/-   ##
==========================================
- Coverage   83.51%   83.45%   -0.06%     
==========================================
  Files          36       36              
  Lines        6835     6895      +60     
==========================================
+ Hits         5708     5754      +46     
- Misses        898      907       +9     
- Partials      229      234       +5     

Impacted Files	Coverage Δ
auth0/resource_auth0_action.go	87.03% <76.92%> (-1.56%)	⬇️
auth0/resource_auth0_organization.go	80.09% <0.00%> (+0.09%)	⬆️
auth0/resource_auth0_user.go	73.72% <0.00%> (+0.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e7f2b4...2a12da9. Read the comment docs.

[willvedd]

Definitely an important enhancement that will prevent unintended destruction. Please excuse the volume of feedback, though it mostly revolves around a couple of minor points.

[willvedd]

What is the reasoning behind casting these secrets to []interface{}? Especially since these arguments get casted into a map[string]interface{} below. As an actionable takeaway, I think I'd prefer to see the type casting consolidated into a singular step and/or the parameter types made to be more explicit. Otherwise, it's difficult to follow the flow of logic.

[sergiught]

What is the reasoning behind casting these secrets to []interface{}?

The secrets are of type schema.TypeList, and that converts to a []interface{} within Go.

Especially since these arguments get casted into a map[string]interface{} below.

They do not, it's their elements that get casted to a map[string]interface{}.

As an actionable takeaway, I think I'd prefer to see the type casting consolidated into a singular step and/or the parameter types made to be more explicit. Otherwise, it's difficult to follow the flow of logic.

We can't, because the secrets look like this actually:

secrets := []interface{}{
  map[string]interface{}{
    "name": "secretName",
    "value": "secretValue",
  },
} 
// secrets is a []interface{} with len 1
// it has an element of type map[string]interface{}

So first we need to typecast the most outer object and then again typecast each element of the slice.

[willvedd]

I'm still figuring out the logic below, but not sure about setting the length to the number of secrets on remote. Couldn't the number of secrets in old and new be greater than the number on remote, thus making this code susceptible to out-of-bounds errors?

[sergiught]

If we specify a capacity when using make, that doesn't mean we can't exceed the capacity on items such as slices or maps, so we won't get any out of bounds error. Go behind the scenes knows to automatically increase the size if we need to add more items. Usually you wouldn't set the capacity if you don't know at that point in time the size of your item.

[willvedd]

What does "old" and "new" mean in this context? Is it the union of current, added and removed? If we're just trying to calculate all secrets that are and were managed, it might not make sense to continue to separate by "new" and "old" beyond this point because that really won't impact the logic below.

My suggestion, assuming my understanding is correct, is to consolidate these into a single slice to simplify the logic below.

[sergiught]

Sure that makes sense.

• Why do we need to get the change and not just the actual current secrets?
• If we use d.Get() we'll just get the secrets that are currently in the config, that means if we remove 1 we won't fetch it, but if we don't and the secret is actually on the action, we'll think that we're trying to wipe something that shouldn't get wiped, when actually we do want to wipe it. Hope this makes sense. Lmk if you have questions.

I applied the feedback and PR got re-reviewed.