Repository navigation
fix: await hasConfigurationChanged before deciding to update the configuration - #77
Open
sarankumar1325 wants to merge 1 commit into
Open
sarankumar1325 wants to merge 1 commit into
sarankumar1325 wants to merge 1 commit into
Conversation
…iguration hasConfigurationChanged() is async, but run() called it without await. The pending Promise is always truthy, so UpdateFunctionConfiguration ran on every deploy, and with dry-run enabled the action returned at "Configuration updates are not simulated" without ever doing the DryRun code update. Await the result and rebuild dist/. Add tests that go through run() with the real comparison: unchanged config skips the update, a changed input still updates, and dry run reaches the DryRun code update. Fixes aws-actions#76
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: Fixes #76
Description of changes:
hasConfigurationChanged()isasync, butrun()called it withoutawait.configChangedwas therefore always a pending Promise, which is truthy, so:UpdateFunctionConfigurationand the update waiter ran on every deploy, even when the configuration already matched the inputs. The README says it is skipped in that case.dry-run: true, the action always stopped at[DRY RUN] Configuration updates are not simulated in dry run modeand returned before theDryRuncode update, so the code was never validated.Changes:
index.js:const configChanged = await hasConfigurationChanged(...).dist/index.js: rebuilt withnpm run build. The only change is the same one-lineawait.New
__tests__/run_config_change_detection.test.js. It drivesrun()with the realhasConfigurationChanged(); the existingrun()tests mock it with a plain boolean, which is why they missed this bug. The tests check that:No configuration changes detectedand skipsUpdateFunctionConfigurationCommand.UpdateFunctionConfigurationCommand.UpdateFunctionCodeCommandwithDryRun: true.On
main, the first and third tests fail and the second passes.Testing:
npx jestpasses (17 suites, 246 tests).npm run lintgives 0 errors; the 2 warnings it prints are already onmainincontainer_image_support.test.js.The false-positive comparisons from #50 / #62 are a separate problem and are not changed here.
I used an AI assistant to write this change. I reviewed it and ran the tests above myself.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.