Skip to content
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

Adds support for custom metadata for aws-elixir. #117

Merged

Conversation

jhosteny
Copy link
Contributor

This adds support for custom metadata for elixir (see #92). It also adds code that was removed in #103 which appears to have inadvertently removed erlang support, as well as functions necessary for elixir support (e.g., collect_request_headers_parameters).

jhosteny added a commit to thoroai/aws-elixir that referenced this pull request Nov 19, 2024
This is only necessary until aws-beam#201 and
aws-beam/aws-codegen#117 are merged, and a new
release of the aws-elixir library is done.
jhosteny added a commit to thoroai/aws-elixir that referenced this pull request Nov 19, 2024
This is only necessary until aws-beam#201 and
aws-beam/aws-codegen#117 are merged, and a new
release of the aws-elixir library is done.
@onno-vos-dev
Copy link
Member

Change looks good 👍 Can you update the tests? Then I'm happy to merge and roll out a new release for aws-elixir the day after merge 👍

@jhosteny
Copy link
Contributor Author

Hi @onno-vos-dev, once #118 is merged, I can rebase the latest version of my changes (with a test) on master, or merge master back in if you prefer that.

@onno-vos-dev
Copy link
Member

Hi @onno-vos-dev, once #118 is merged, I can rebase the latest version of my changes (with a test) on master, or merge master back in if you prefer that.

Pick your poison :-) I prefer rebase but I won't stop you from merging master back either 👍 Whatever you find easiest.

This adds support for custom metadata for elixir (see aws-beam#92). It also
adds code that was removed in aws-beam#103 which appears to have inadvertently
removed erlang support, as well as functions necessary for elixir
support (e.g., `collect_request_headers_parameters`).
The shortest service definition which uses prefix headers is AWS
DataExchange. It is several thousand lines of generated code, so we keep
the expected generated output in a file.
@jhosteny jhosteny force-pushed the feat/add-custom-metadata-support branch from 6b50f2a to d559fd9 Compare November 20, 2024 14:02
@jhosteny
Copy link
Contributor Author

Hi @onno-vos-dev , here you go. I added the test as a second commit so you can review that separately. The first commit is the same, other than the change in sha. Note that the generated code for the service that shows the custom headers is ~3000 lines, so I elected to put that into a file.

@onno-vos-dev
Copy link
Member

onno-vos-dev commented Nov 20, 2024

Please update the data_exchange.ex module to the one I sent over Elixir slack 👍 Github doesn't like me attaching the file here and somehow I cannot push to your branch 😞

Alternatively, generate it yourself and add it here 👍

Otherwise LGTM! 🚢

@jhosteny
Copy link
Contributor Author

Hi @onno-vos-dev , I'm just adding this for posterity. The failure appeared to be from me running a later version of OTP / elixir, and generating the fixture with that. I am amending the PR to use the version you sent me, and will verify it works locally before pushing. The fundamental issue appears to be that the maps being loaded are not in the same key order across elixir versions.

@onno-vos-dev onno-vos-dev merged commit feec7b3 into aws-beam:master Nov 20, 2024
3 checks passed
@onno-vos-dev
Copy link
Member

@jhosteny Thank you! I'll set a calendar reminder for tomorrow to roll out a new aws-elixir release 👍 I'd prefer the nightly job to take care of that on the aws-elixir side 👍

If you need it today let me know and I'll push the generated code manually 👍

@jhosteny
Copy link
Contributor Author

Take your time, thanks!

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.

2 participants