Skip to content

data source depends_on#26375

Merged
jbardin merged 1 commit intomasterfrom
jbardin/data-force-plan-read
Sep 25, 2020
Merged

data source depends_on#26375
jbardin merged 1 commit intomasterfrom
jbardin/data-force-plan-read

Conversation

@jbardin
Copy link
Copy Markdown
Member

@jbardin jbardin commented Sep 25, 2020

A data source referencing another data source through depends_on should
not be forced to defer until apply. Data sources have no side effects,
so nothing should need to be applied. If the dependency has a
planned change due to a managed resource, the original data source will
also encounter that further down the list of dependencies.

This prevents a data source being read during plan for any reason from
causing other data sources to be deferred until apply. It does not
change the behavior noticeably in 0.14, but because 0.13 still had
separate refresh and plan phases which could read the data source, the
deferral could cause many things downstream to become unexpectedly
unknown until apply.

@jbardin jbardin requested a review from a team September 25, 2020 14:36
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 25, 2020

Codecov Report

Merging #26375 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
terraform/eval_read_data_plan.go 82.66% <100.00%> (+0.47%) ⬆️
internal/providercache/dir.go 66.66% <0.00%> (-6.25%) ⬇️
terraform/node_resource_plan.go 92.66% <0.00%> (-1.84%) ⬇️
backend/remote/backend_common.go 52.51% <0.00%> (+0.71%) ⬆️

for _, d := range n.dependsOn {
if d.Resource.Mode == addrs.DataResourceMode {
// Data sources have no side effects, so don't create a need to
// delay this read. If the they do have a change planned, it must
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// delay this read. If the they do have a change planned, it must
// delay this read. If they do have a change planned, it must

typo!

A data source referencing another data source through depends_on should
not be forced to defer until apply. Data sources have no side effects,
so nothing should need to be applied. If the dependency has a
planned change due to a managed resource, the original data source will
also encounter that further down the list of dependencies.

This prevents a data source being read during plan for any reason from
causing other data sources to be deferred until apply. It does not
change the behavior noticeably in 0.14, but because 0.13 still had
separate refresh and plan phases which could read the data source, the
deferral could cause many things downstream to become unexpectedly
unknown until apply.
@jurgenweber
Copy link
Copy Markdown

jurgenweber commented Oct 7, 2020

for me 0.13.4 half resolved it for me reading your description I did some experiments.

I have module a and module b with many data 'aws_iam_policy_document' resources, created in for_each loops. When I originally created this in 0.13.2, every time I planned (irrelevant of what code I was changing). I was getting a diff for all of them, looking like this:

 # module.applications["0"].data.aws_iam_policy_document.this_s3["0"] will be read during apply
 # (config refers to values not yet known)
 <= data "aws_iam_policy_document" "this_s3"  {
      ~ id      = "3763867991" -> (known after apply)
      ~ json    = jsonencode(
            {
              - Statement = [
                  - {
                      - Action   = [
                          - "s3:ListAllMyBuckets",
                          - "s3:GetBucketLocation",
                        ]
                      - Effect   = "Allow"
                      - Resource = "arn:aws:s3:::*"
                      - Sid      = ""
                    },
                  - {
                      - Action   = "s3:ListBucket"
                      - Effect   = "Allow"
                      - Resource = "arn:aws:s3:::myapp-dev-sydney-aws"
                      - Sid      = ""
                    },
                  - {
                      - Action   = [
                          - "s3:PutObject",
                          - "s3:GetObject",
                          - "s3:AbortMultipartUpload",
                        ]
                      - Effect   = "Allow"
                      - Resource = "arn:aws:s3:::myapp-dev-sydney-aws/*"
                      - Sid      = ""
                    },
                ]
              - Version   = "2012-10-17"
            }
        ) -> (known after apply)
      - version = "2012-10-17" -> null

You can see my original post on what I was experiencing here: https://discuss.hashicorp.com/t/data-aws-iam-policy-document-and-for-each-showing-changes-on-every-plan-and-nothing-on-apply/14606

when I upgraded to 0.13.4 we noticed that it disappeared but then would come back sometimes.

I identified that it only came back when we made a change in module a which seemed odd because it is completely unrelated but then I noticed that there was a dependency on module b for module a. I thought about this for a moment and decided it wasn't necessary.

Now everything seems resolved, but maybe still a terraform bug here. if I make changes in module a, I do not get the diffs (as shown above) for all of the data source aws_iam_policy_documents that are created and maintained in module b but only after removing the depends_on key for module a, on module b. 👍

Thanks

@ghost
Copy link
Copy Markdown

ghost commented Oct 26, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 26, 2020
@jbardin jbardin deleted the jbardin/data-force-plan-read branch November 20, 2020 20:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants