Skip to content

Upgrade localstack #1596

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

Closed
wants to merge 4 commits into from
Closed

Upgrade localstack #1596

wants to merge 4 commits into from

Conversation

stof
Copy link
Member

@stof stof commented Oct 25, 2023

This upgrades the localstack image used for integration test to the current latest version instead of a very old version.

The goal is to try fixing #1489 which had a failed CI due to a parity mismatch between localstack and AWS for Route53. The bug has been fixed in moto (used by localstack) and I don't know yet whether this is enough to fix the issue entirely in localstack or whether it requires additional changes there (the latest localstack version already uses a version of moto that includes the fix).

Future changes might involve investigating whether our S3 and SQS clients might rely on localstack to run integration tests instead of the testing-s3 and testing-sqs docker images for which we (don't) maintain a fork in this organization (and which rely on tools that have no activity since 2019)

@stof
Copy link
Member Author

stof commented Oct 25, 2023

And now, I need to debug why some integration tests failed in CI and not locally...

@GrahamCampbell
Copy link
Contributor

Yeh, I pinned this a while ago because localstack introduced some bugs.

@stof
Copy link
Member Author

stof commented Oct 25, 2023

@GrahamCampbell the weird thing in my case is that things worked locally (unless something is broken in make clean making it not clean things properly and so 0.14.2 would still be used when starting containers again)

@stof
Copy link
Member Author

stof commented Oct 25, 2023

And this is precisely an attempt at pinning a newer version because the old version also has bugs.

@stof stof mentioned this pull request Oct 26, 2023
stof added 3 commits October 26, 2023 17:23
The new localstack version emulates AWS better, returning a consumed
capacity only when the request asks for it.
@stof stof force-pushed the update_localstack branch from 690b59b to 79c45bd Compare October 26, 2023 15:23
@stof
Copy link
Member Author

stof commented Oct 26, 2023

The failure in the test for CreateHostedZone might actually indicate an actual bug in the way empty lists are populated when parsing XML responses. I will check it in more details.

I also need to investigate the failure in the test for ChangeResourceRecordSets to see whether it is a bug in localstack or an issue in our testsuite (maybe our tests are not properly isolated)

In the new localstack version, creating a zone automatically creates the
NS and SOA records for that zone, emulating what actually happens on
AWS.
@stof
Copy link
Member Author

stof commented Oct 27, 2023

OK, I found why I was not seeing failures locally: the clean commands of the Makefile setup were doing a partial clean. So initializing it again would reuse the previous container by starting it again, ignoring any update of the image being pulled.
I submitted #1597 to fix this.

The failure from the test for ChangeResourceRecordSets has been fixed by updating the test. The new version of localstack emulates Route53, which implies that a created zone automatically has NS and SOA records which are mandatory in a zone. And so the assertion on the number of listed records needs to account for them.
For the remaining failure, I reported it at localstack/localstack#9486. When trying to track it down, localstack 1.0 was already affected. This might have been the issue that made us pin to 0.14 initially (which would be a bad news if it was not reported upstream at that time).

@stof stof mentioned this pull request Nov 10, 2023
@GrahamCampbell
Copy link
Contributor

I vote that we skip the failing tests and then merge this. I am happy to make the code changes to do that, if anyone else is pushed for time. ❤️

@GrahamCampbell
Copy link
Contributor

FYI, v3.0.0 has been tagged on their GitHub repo, so possibly that will be formally released in the next few hours or days. We could do that upgrade here, or do it on the SQS JSON PR.

@stof
Copy link
Member Author

stof commented Nov 16, 2023

As said above, I started doing the update. I will finish it either this evening or Saturday.
My branch skips the integration tests for now. But for unit tests, I'm properly updating them to assert the json instead of the old data.

@jderusse jderusse closed this Nov 22, 2023
@stof stof deleted the update_localstack branch December 18, 2023 08:59
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.

3 participants