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

Don't error on already mapped tokens #1420

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Oct 9, 2023

Token mapping should not error when it is unable to map a token that was mapped manually. Apparently, this is not a common scenario, but I encountered it as part of pulumi/pulumi-spotinst#436.

Fixes #1421

@iwahbe iwahbe self-assigned this Oct 9, 2023
@iwahbe iwahbe requested a review from a team October 9, 2023 16:07
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Diff for pulumi-random with merge commit 859afc4

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Diff for pulumi-azuread with merge commit 859afc4

@iwahbe iwahbe force-pushed the iwahbe/dont-error-on-already-mapped-tokens branch from 8f2f286 to 1b52fa6 Compare October 9, 2023 16:10
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Diff for pulumi-random with merge commit d1a7b78

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Diff for pulumi-azuread with merge commit d1a7b78

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2023

Codecov Report

Merging #1420 (1b52fa6) into master (e3aabd3) will decrease coverage by 4.03%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##           master    #1420      +/-   ##
==========================================
- Coverage   61.87%   57.84%   -4.03%     
==========================================
  Files         182      282     +100     
  Lines       32708    39476    +6768     
==========================================
+ Hits        20238    22835    +2597     
- Misses      11332    15307    +3975     
- Partials     1138     1334     +196     
Files Coverage Δ
pkg/tfbridge/tokens/known_modules.go 73.68% <64.70%> (+4.29%) ⬆️

... and 103 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM. I'm a bit miffed did not catch this at plan/review time. Having ComputeTokens never override the tokens manually specified is a very important property as this is our "user override" story - actually can you elaborate a bit how did this behave before the PR?

@t0yv0
Copy link
Member

t0yv0 commented Oct 9, 2023

Would there be a situation where this change would cause a change in token allocation? As opposed to simply turning failing programs into working programs?

@iwahbe
Copy link
Member Author

iwahbe commented Oct 9, 2023

LGTM. I'm a bit miffed did not catch this at plan/review time. Having ComputeTokens never override the tokens manually specified is a very important property as this is our "user override" story - actually can you elaborate a bit how did this behave before the PR?

Before this PR, we would error on missing modules for already mapped tokens:

"pkg_mod_res" would error unless "mod" was in the list of mapped modules. This would happen regardless of if "pkg_mod_res" was manually mapped. In effect, we would return an error when we were unable to compute information that we would end up not needing (the module for a manually mapped resource).

Would there be a situation where this change would cause a change in token allocation? As opposed to simply turning failing programs into working programs?

I don't expect so. We will not experience any mapping changes from this PR.

It is always possible that user's will rely on the incorrect behavior, but I can't imagine a sensible usage that will break with this change.

@iwahbe iwahbe merged commit 2654ed6 into master Oct 9, 2023
9 checks passed
@iwahbe iwahbe deleted the iwahbe/dont-error-on-already-mapped-tokens branch October 9, 2023 16:42
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.

Automatic token mapping errors when it cannot map a manually mapped token
4 participants