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

Add token mapping strategies with inferred fallback #2882

Merged

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Feb 3, 2025

This PR adds two new token mapping strategies for providers which add a fallback to the InferredModules strategy:

  • KnownModulesWithInferredFallback is equivalent to KnownModules but with a fallback
  • MappedModulesWithInferredFallback is equivalent to MappedModules but with a fallback

This should make token mapping for all providers easier since we can migrate all KnownModules providers to KnownModulesWithInferredFallback and all MappedModules to MappedModulesWithInferredFallback

The two strategies are written to make automated migrations using gopatch easy and are separated in a different module.

Related to pulumi/ci-mgmt#1347

@VenelinMartinov VenelinMartinov requested review from iwahbe and a team February 3, 2025 18:31
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 73.80952% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.76%. Comparing base (a801f99) to head (5f80504).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
...tfbridge/tokens/fallbackstrat/fallback_strategy.go 73.80% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2882      +/-   ##
==========================================
+ Coverage   67.74%   67.76%   +0.02%     
==========================================
  Files         328      329       +1     
  Lines       42116    42176      +60     
==========================================
+ Hits        28531    28581      +50     
- Misses      11994    12003       +9     
- Partials     1591     1592       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Is there a reason we can't expose a much simpler tokens.CombineStrategies(strategies ...Strategy) Strategy api?

The tokens.CombineStrategies would just try each strategy in turn until one works.

KnownModulesWithInferredFallback and MappedModulesWithInferredFallback seems like specialization that adds complexity.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Feb 3, 2025

We could but that'd expose a wider public area which we are committing to maintaining. The WithInferredFallback variants are our only current use case for CombineStrategies and expose a much narrower surface area. This will in turn allow us to

  • refactor as we see fit
  • Expose CombineStrategies in the future if needed

In addition, because some of the config is shared, we can use the first strategy inputs to configure the InferredModules strat and automate the whole migration for all providers

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

Can you summarize what this change does, and when it would apply?

Holding for @iwahbe to respond to your comment.

Thank you for all these improvements!

@iwahbe
Copy link
Member

iwahbe commented Feb 4, 2025

We could but that'd expose a wider public area which we are committing to maintaining. The WithInferredFallback variants are our only current use case for CombineStrategies and expose a much narrower surface area. This will in turn allow us to

  • refactor as we see fit
  • Expose CombineStrategies in the future if needed

We are already committed to exposing the strategy for KnownModules, MappedModules and InferredModules. The additional overhead to exposing an or combinator is extremely minimal. IMO it is less overhead then exposing *WithInferredFallback functions.

In addition, because some of the config is shared, we can use the first strategy inputs to configure the InferredModules strat and automate the whole migration for all providers

That design makes me pretty uncomfortable. Field X is required unless it is being passed to a function that will fill it in leads to a very confusing space.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Looking at this API, I'm wondering if we want to distinguish error types. The safest thing to do would be to introduce a StrategyDoesNotApply error, and then only forward calls when the strategy returns an error that wraps that.

This would allow stratagies to signal critical failures vs simply not applying.

pkg/tfbridge/tokens/known_modules.go Outdated Show resolved Hide resolved
@VenelinMartinov VenelinMartinov force-pushed the vvm/token_mapping_strats_with_inferred_fallback branch 2 times, most recently from 96c5241 to de71ddd Compare February 4, 2025 15:24
@VenelinMartinov VenelinMartinov force-pushed the vvm/token_mapping_strats_with_inferred_fallback branch from de71ddd to 1b54efe Compare February 5, 2025 10:25
@VenelinMartinov VenelinMartinov force-pushed the vvm/token_mapping_strats_with_inferred_fallback branch from a2dd441 to cac2b8e Compare February 5, 2025 11:16
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

2 nits in the documentation, then LGTM

pkg/tfbridge/tokens/fallbackstrat/fallback_strategy.go Outdated Show resolved Hide resolved
pkg/tfbridge/tokens/fallbackstrat/fallback_strategy.go Outdated Show resolved Hide resolved
@VenelinMartinov VenelinMartinov enabled auto-merge (squash) February 6, 2025 13:48
@VenelinMartinov VenelinMartinov merged commit 8c8e7f0 into master Feb 6, 2025
71 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/token_mapping_strats_with_inferred_fallback branch February 6, 2025 14:13
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.103.0.

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.

4 participants