Add back ecr.getCredentials data source from v6 - #6590
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcfd4ffe3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Does the PR have any schema changes?Generated by schema-tools v0.8.1. Looking good! No breaking changes found. New functions:
Maintainer note: consult the runbook for dealing with any breaking changes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6590 +/- ##
=======================================
Coverage 15.84% 15.84%
=======================================
Files 376 376
Lines 91883 91883
=======================================
Hits 14558 14558
Misses 77304 77304
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review Verdict: Comment OnlyImplementation is clean, well tested, and verified faithful to v6 semantics (id==registry_id, required registryId, non-secret authorizationToken), with all currently-reported CI checks green — but this re-adds a public Pulumi data source across all generated SDKs, and the PR description itself acknowledges full v6 parity isn't 100% verified yet, so a human should own the compatibility contract before it ships. View session · Was this review helpful? Yes · No |
![eon-pulumi-agent[bot]](https://avatars.githubusercontent.com/in/4080224?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the ECR getCredentials back-compat shim: the new addLegacyECRCredentialsDataSource wrapper in provider/resources.go, the bridge Field overrides (registryId required, region omitted, authorizationToken not-secret), the regenerated schema/SDKs for all five languages, and the new acceptance/upgrade tests in examples/examples_yaml_test.go.
Verified against the actual v6.66.2 release (schema + the original patches/0009-Add-ECR-credentials_data_source.patch): the shim correctly reproduces v6's id == registry_id semantics (v6 set id from registry_id; the new upstream aws_ecr_authorization_token sets id from region), keeps registryId required, and keeps authorizationToken un-marked as a Pulumi secret exactly as v6 did. The extra userName/password outputs are additive (not present in v6) and don't break existing v6 programs. go build on the provider package succeeds, and all currently-reported CI checks (schema build, compare_sdk/build_sdk for all five languages, license check) are green; a few test_provider/build_provider/upstream_tests jobs were still running at review time with no failures.
No bugs, security, or correctness issues that would block merge were found. Three non-blocking nits below: the new example fixture's directory placement deviates from the documented examples/test-programs/<name> convention, authorizationToken and password protect the same underlying credential but only one is marked secret (worth a doc/comment clarification), and the new acceptance test never actually asserts that authorizationToken/userName remain non-secret (so a future regression there wouldn't be caught).
This PR reintroduces a public Pulumi data source (aws.ecr.getCredentials) across all generated SDKs. Once released, downstream users will begin depending on it again, and the PR description itself flags that full v6 parity is not yet 100% verified ("if there are some compatibility issues then we will fix forward") -- that combination of a public API re-addition and an author-acknowledged compatibility risk is the kind of one-way door worth a human sign-off on the exact compatibility contract, even though the implementation itself looks correct and is well tested.
There was a problem hiding this comment.
Re-reviewed the follow-up commit (56b17d7f2, "updates based on eon review"). All three nits from the previous round are addressed:
- The
ecr-credentialsexample program moved toexamples/test-programs/ecr-credentials/, and bothTestAccEcrCredentialsandTestEcrCredentialsUpgradenow reference it viafilepath.Join(..., "test-programs", "ecr-credentials"), matching the convention used by every other fixture in this file. provider/resources.gonow has a code comment on theauthorization_tokenField override explaining it's intentionally non-secret for v6 compatibility even though it base64-encodes the same credential as the (secret)passwordfield.validateEcrCredsnow assertsresult.Outputs["userName"].Secretisfalse, closing the test-coverage gap on that compatibility guarantee.
No new issues in the incremental diff, which is limited to the rename and these three small additions. CI is green (schema build, compare_sdk/build_sdk across all five languages, test_provider, license check), with only a couple of build_provider/upstream_tests jobs still in progress and nothing failing.
In the v7 release we removed the old `ecr.getCredentials` data source. This datasource did not exist in upstream Terraform, it was added as a new datasource in a patch. Upstream added their own official datasource to achieve the same functionality `ecr.getAuthorizationToken`. This datasource is better in several ways and gives the same functionality. To give users extra time to migrate we are adding back the old `getCredentials` datasource, but we are doing it by adding a shim/proxy to the `getAuthorizationToken` data source. The specific customizations that we have made on top are to keep complete backwards compatibility with the v6 version of `getCredentials`.
56b17d7 to
03e9f89
Compare
|
This PR has been shipped in release v7.41.0. |
3 participants
In the v7 release we removed the old
ecr.getCredentialsdata source. This datasource did not exist in upstream Terraform, it was added as a new datasource in a patch. Upstream added their own official datasource to achieve the same functionalityecr.getAuthorizationToken. This datasource is better in several ways and gives the same functionality.To give users extra time to migrate we are adding back the old
getCredentialsdatasource, but we are doing it by adding a shim/proxy to thegetAuthorizationTokendata source.The specific customizations that we have made on top are to keep complete backwards compatibility with the v6 version of
getCredentials.Change Type
provider/)upstream/,patches/,scripts/upstream.sh).ci-mgmt.yaml)Risk
The main risk is that this is not 100% compatible with the v6 version of the datasource. We've added upgrade tests to validate that it is. If there are some compatibility issues then we will fix forward.