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

fix: resolve blank repo metadata #2583

Merged
merged 1 commit into from
Jul 21, 2023
Merged

fix: resolve blank repo metadata #2583

merged 1 commit into from
Jul 21, 2023

Conversation

hugorut
Copy link
Contributor

@hugorut hugorut commented Jul 21, 2023

Resolves issue where repo metadata is blank for infracost comment. This fixes a regression that was introduced with the ContextValues concurrency fix. Prior to the concurrency fix RunContext Values was returning the value of the underlying map and not a copy for it. So setting ctxValues['repoMetadata'] actually altered the RunContext Values. Hence why the old code which set the key before creating a clone actually worked. The old code with comments for better understanding:

// This line actually alters ctx.Values as well as the local variable.
ctxValues["repoMetadata"] = metadata

if ctx.IsInfracostComment() {
	ctxValues = make(map[string]interface{}, len(ctxValues))
        // now ctx.ctx.GetContextValues returns the altered map so the clone contains the "repoMetadata" key.
	for k, v := range ctx.GetContextValues() {
		ctxValues[k] = v
	}
	ctxValues["command"] = "comment"
}

once this was changed to return a map copy ctxValues this meant:

// This line only modified the local variable.
ctxValues["repoMetadata"] = metadata

if ctx.IsInfracostComment() {
	ctxValues = make(map[string]interface{}, len(ctxValues))
	// Then Values returns the unaltered ctx values (the correct behaviour).
	for k, v := range ctx.ContextValues.Values() {
		ctxValues[k] = v
	}
	ctxValues["command"] = "comment"
}

So repoMetadata was removed.

This fix removes the unecessary ctxValues copy (as it is already a local copy) and instead just sets the command to be the correct string.

Resolves issue where repo metadata is blank for `infracost comment`. This fixes a regression that was introduced with the `ContextValues` concurrency fix. Prior to the concurrency fix RunContext Values was returning the value of the underlying map and not a copy for it. So setting `ctxValues['repoMetadata']` actually altered the RunContext Values. Hence why the old code which set the key before creating a clone actually worked. The old code with comments for better understanding:

```go
// This line actually alters ctx.Values as well as the local variable.
ctxValues["repoMetadata"] = metadata

if ctx.IsInfracostComment() {
	ctxValues = make(map[string]interface{}, len(ctxValues))
        // now ctx.ctx.GetContextValues returns the altered map so the clone contains the "repoMetadata" key.
	for k, v := range ctx.GetContextValues() {
		ctxValues[k] = v
	}
	ctxValues["command"] = "comment"
}
```

once this was changed to return a map copy `ctxValues` this meant:

```go
// This line only modified the local variable.
ctxValues["repoMetadata"] = metadata

if ctx.IsInfracostComment() {
	ctxValues = make(map[string]interface{}, len(ctxValues))
	// Then Values returns the unaltered ctx values (the correct behaviour).
	for k, v := range ctx.ContextValues.Values() {
		ctxValues[k] = v
	}
	ctxValues["command"] = "comment"
}
```

So `repoMetadata` was removed.

This fix removes the unecessary ctxValues copy (as it is already a local copy) and instead just sets the command to be the correct string.
@hugorut hugorut self-assigned this Jul 21, 2023
@hugorut hugorut requested a review from aliscott July 21, 2023 14:49
@hugorut hugorut merged commit d47b3bc into master Jul 21, 2023
9 checks passed
@hugorut hugorut deleted the fix/repo-metadata branch July 21, 2023 14:54
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.

None yet

2 participants