Skip to content

fix: add sync state and context to check previous run details#3989

Merged
mndeveci merged 10 commits intoaws:developfrom
mndeveci:sync_context_and_state
Jun 23, 2022
Merged

fix: add sync state and context to check previous run details#3989
mndeveci merged 10 commits intoaws:developfrom
mndeveci:sync_context_and_state

Conversation

@mndeveci
Copy link
Contributor

@mndeveci mndeveci commented Jun 22, 2022

Which issue(s) does this change fix?

N/A

Why is this change necessary?

If customer runs sam sync with a following sam sync --no-dependency-layer for a Java function without making a code change, the second run will fail since cached build will kick in and use the package which doesn't have dependencies in it (since previous run was using ADL which omits the dependencies). This issue only happens for Java runtimes, since we exclude dependencies after build is completed.

How does it address the issue?

By introducing "Sync State" which will keep track of used options in previous session so that sam sync can check if there were any changes and run cleaning operation before starting the sync.

What side effects does this change have?

If customer runs sam sync (with ADL) and then switches to sam sync --no-dependency-layer (without ADL), their cached/incremental builds will be cleaned up so a full build needs to be completed. Sub-sequent runs after this will have cached/incremental builds.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added pr/internal area/sync sam sync command labels Jun 22, 2022
@mndeveci mndeveci marked this pull request as ready for review June 23, 2022 01:02
Copy link
Contributor

@qingchm qingchm left a comment

Choose a reason for hiding this comment

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

To capture a few discussion points mentioned in the code review session:

  1. The wrapping of the withs for context creation in sync seems to have too many layers, it would be nice to have it refactored to be cleaner. However the shared temp dirs and template file location between contexts, as well as the scheduled delays needs to be preserved, so we need to be careful about it. It would be nice to have a separate PR to address it, but for current release scope we will play safe and leave it as it is right now.
  2. As the current behaviour of the sync state stores the dependency layer's status, we explored the possibility of auto-saving this into samconfig.toml. That would help in the scenario where the user deployed the initial application with no-dependency-layer but forget to add this flag in later executions, current solutions will not fill in the previous option value, but instead safely switch to ADL under this scenario. A suggested solution is that after each execution we prompt the user if they want to store the setting into toml. But after discussion with UX, this improvement will be addressed by environment and is not in this PR's scope.
  3. Locks will protect one process's thread safety, since for each process only one sync state instance will be instantiated, we will not need of locking mechanism on toml write even though it's not atomic. As for write errors, we will not be catching it since this is consistent with how we do it for build.toml, currently we don't see any issues/dangers.

@mndeveci mndeveci enabled auto-merge (squash) June 23, 2022 19:47
@mndeveci mndeveci merged commit 078192a into aws:develop Jun 23, 2022
@mndeveci mndeveci deleted the sync_context_and_state branch June 23, 2022 23:55
sidhujus pushed a commit to sidhujus/aws-sam-cli that referenced this pull request Jul 6, 2022
* fix: add sync state and context to check previous run details

* add unit tests

* fix unit tests & formatting

* add integration tests

* remove redundant initialization

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
moelasmar added a commit that referenced this pull request Jul 14, 2022
* chore: fix canary issues (#3978)

* Update sam logs setup to align with function update

* fix: don't use same layername in the template otherwise they will override themselves

* fix build tests

* downgrade python version for windows tests & add missing fields for adl sync tests

* re-format

* chore: Clarify debug log message in build_context command (#3968)

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* fix: clean build folder before starting sync operation (#3982)

* fix: clean build folder before starting sync operation

* use function or layer build folder

Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>

* fix: Updated default option to Y for sync and added green colour to success message (#3986)

Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

* fix: Use cache build instead of incremental when building in container (#3988)

* use cache build instead of incremental when building in container

* use self reference

* bug: make sure all pullers in child combine puller also get stopped (#3990)

make sure all pullers in child combine puller also get stopped

* feat: Add --resource option input validation for sync --code (#3992)

* Add resource type validation for sync --code

* Refactor to use click.Choice and fix integration tests

* Add help text in option description

* Update samcli/commands/sync/command.py

Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>

* Remove debug flag for clean integ test output

Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>

* chore: replace InvokeContextException with granular ones (#3966)

* chore: replace InvokeContextException with granular ones

* but InvokeContextException back and extend other classes from it

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* chore(deps): bump Newtonsoft.Json in dotnet example (#3996)

Bumps [Newtonsoft.Json](https://github.com/JamesNK/Newtonsoft.Json) from 12.0.1 to 13.0.1.
- [Release notes](https://github.com/JamesNK/Newtonsoft.Json/releases)
- [Commits](JamesNK/Newtonsoft.Json@12.0.1...13.0.1)

---
updated-dependencies:
- dependency-name: Newtonsoft.Json
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com>

* fix: add sync state and context to check previous run details (#3989)

* fix: add sync state and context to check previous run details

* add unit tests

* fix unit tests & formatting

* add integration tests

* remove redundant initialization

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* chore: Update lambda builder version to 1.18.0 (#3999)

* Update lambda builder dependency version

* Update reproducible-linux hash

* feat: Adding --s3-bucket to sam sync (#3977)

* Added --s3-bucket option to sam sync

* Updated base command for integ tests

* Changed s3_bucket argument to be optional string

* Changed --s3-bucket to use existing util and refactor to ignore callbacks

* Removed callback argument for s3_bucket_option()

* Reverted function back and refactor argument name

* Moved logic out of arguments

* Added integ test to check if --s3-bucket option is being used

* Removed bucket check and replacede with resource check

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

* chore: fix java integration tests to use java8 runtime (#4002)

* chore: fix java integration tests to use java8 runtime

* update pom.xml definition as well

Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com>

* fix: add try/except for parse_time which is used by logs and traces commands (#4006)

* fix: issues with parallel build when using compiled languages (#4008)

* fix: run layer builds and function builds in separate async contextx

* add passthrough methods

* fix method call

* update unit tests

* add integration tests

* remove comment

* feat: updating app templates repo hash with (b057f88e4d4fe04acecbc22bab6b12cfbf695761) (#4007)

Co-authored-by: GitHub Action <action@github.com>

* chore: bump version to 1.53.0 (#4014)

* chore: skip TestSyncAdlWithWatchStartWithNoDependencies on windows (#4013)

Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* feat: remove accelerate beta experimental flag (#3973) (#3981)

* chore: remove accelerate beta experimental flag (#3973)

* remove beta experimental flag

* fix the if check for name

* use text variable directly

* fix unit tests

* fix unit test and remove accelerate flags

* remove accelerate beta feature flag

* skip accelerate experimental test

* fix build tests

Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

* fix integration tests

* fix unit and integration tests

* reformat

* docs: Update logs and traces docs (#4009)

* Update logs and traces docs

* Fix typos

Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>

* Logging UnicodeDecodeError details in LocalApigwService._request_handler (#4016)

Co-authored-by: Lucas Cimon <ext.lucas.cimon@sncf.fr>

* Fix infinite deploy loop on small stack edge case (#4005)

* Fix typo in doc

* Replace last_event_time with execution_time

To not skip events in case of throttle

* Update tests for execute_changeset and wait_for_execute

* Document execution_time and set a default value

Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>

Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>

* chore: README.md Updated typo fixes done. (#4032)

* README.md Updated 

Updated README.md typo fixes.

* Update README.md

* feat: updating app templates repo hash with (dca20ade1b290262b6457006b52abadeacdddaf8) (#4030)

Co-authored-by: GitHub Action <action@github.com>

* Adding blogposts and workshop material to readme (#3997)

* blogs

* Readme update

* more

* some more

* roadmap

* adding pip

* added powertools

* adding more outbound links

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Update README.md

Co-authored-by: Chris Rehn <crehn@outlook.com>

* Adding sam validate in help context in the CLI.

* updating per Mehmet's feedback

* adding recent launches to readme

* updating readme

* minor updates

* updated - removed beta keyword for accelerate

Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Co-authored-by: Chris Rehn <crehn@outlook.com>
Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>

* chore: Bump SamTranslator version to 1.47.0 (#4051)

* fix: Don't use deps dir for non-cached builds (#4052)

* Don't use deps dir for non-cached builds

* Remove unused import

* Fix test, rename variable to cached

* Make default cached false

* Add function definition

* Remove debug prints

Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Dan Vicarel <shundra8820@gmail.com>
Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com>
Co-authored-by: Lucas <12496191+lucashuy@users.noreply.github.com>
Co-authored-by: mingkun2020 <68391979+mingkun2020@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jacob Fuss <32497805+jfuss@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Lucas Cimon <925560+Lucas-C@users.noreply.github.com>
Co-authored-by: Lucas Cimon <ext.lucas.cimon@sncf.fr>
Co-authored-by: Mickael Bourgois <35171168+BourgoisMickael@users.noreply.github.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
Co-authored-by: Prasad Jivane <prasadjivane@gmail.com>
Co-authored-by: Praneeta Prakash <praneeta.prakash@gmail.com>
Co-authored-by: Chris Rehn <crehn@outlook.com>
Co-authored-by: aws-sam-cli-bot <46753707+aws-sam-cli-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sync sam sync command pr/internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants