Skip to content

Update generated code #1601

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

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Update generated code #1601

merged 3 commits into from
Nov 23, 2023

Conversation

async-aws-bot
Copy link
Collaborator

@async-aws-bot async-aws-bot commented Nov 9, 2023

The AWS API contract changed with version 3.288.1.

This PR contains the new definition for Services.

@stof
Copy link
Member

stof commented Nov 9, 2023

Tests are failing because the Sqs client has migrated to the JSON format instead of the XML format.

For unit tests, it requires updating the tests to expect JSON format
For integration tests, it requires replacing the asyncaws/testing-sqs docker image with an image supporting the JSON format.

and the code generator also has a bug (which is why static analysis fails and is also triggered in some tests)

@GrahamCampbell
Copy link
Contributor

Probably we should also bump the major version, since the exceptions have changed.

@stof
Copy link
Member

stof commented Nov 9, 2023

AFAICT, we only have much more cases that have dedicated exceptions instead of the generic ClientException. As the specific exceptions extend ClientException, I'm not sure it is actually a BC break.

} else {
$payload['Attributes'] = [];
foreach ($v as $name => $mv) {
if (!QueueAttributeName::exists($mapKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$name not $mapKey.

Copy link
Member

Choose a reason for hiding this comment

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

the bot will not automatically fix bugs in the code generator by reading review comments...

@stof
Copy link
Member

stof commented Nov 9, 2023

however, it might make sense to still make it a major version of that service in case some people use alternate implementations of SQS (in local setups for instance) which might not support the json format (which is basically what our own testsuite does)

@stof
Copy link
Member

stof commented Nov 10, 2023

For reference, the official AWS SDK released the switch to the JSON protocol in a patch release (3.285.2), not even in a minor release.

@GrahamCampbell
Copy link
Contributor

I guess we need to skip the localstack sqs tests for now?

Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

Probably we should just re-gen the baseline file for those psalm issues.

@stof
Copy link
Member

stof commented Nov 10, 2023

For the integration tests, we could indeed skip them until #1596 is done and then localstack is used for the SQS integration tests instead of our custom unmaintained image (that does not support json). This will actually also need to wait for localstack to support the json protocol, which is in-progress
For unit tests, we need to update them.
For Psalm, we should indeed put them in the baseline. That's our usual errors for enum fields in json-based services (not sure why we don't have the similar issue for query-based services)

@stof
Copy link
Member

stof commented Nov 11, 2023

I started the work of updating the testsuite for this.

@jderusse
Copy link
Member

PR to add support to LocalStack has just been merged localstack/localstack#8268 a new version should be tagged soon

@stof
Copy link
Member

stof commented Nov 11, 2023

@jderusse but updating localstack in #1596 is currently stuck because it has a bug that impacts our CI (which made us pin an old release years ago, but unfortunately without reporting it upstream at that time). So for now, I'll just skip those tests.

@async-aws-bot async-aws-bot force-pushed the bot-code-update branch 5 times, most recently from 9836cc9 to 8d8df4e Compare November 18, 2023 06:20
@async-aws-bot async-aws-bot force-pushed the bot-code-update branch 2 times, most recently from d8a4ca1 to d530b57 Compare November 22, 2023 06:23
@jderusse
Copy link
Member

ok... I was wrong. SQS test suite used https://github.com/async-aws/testing-sqs (an not localstack)
If I'm not wrong, this container is also used by Symfony to test messenger's bridge.

I've temporary disabled the tests to unlock this situation (this PR become bigger and bigger everyday)

@stof stof merged commit c4a7cfb into async-aws:master Nov 23, 2023
@stof
Copy link
Member

stof commented Nov 23, 2023

I'm merging this so that the bot does not override the manual fixes tomorrow morning.

@jderusse jderusse mentioned this pull request Nov 23, 2023
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