Skip to content

REST XML Request Functionality#123

Merged
mattBrzezinski merged 7 commits into
v1from
MB/rest-xml-service
Jun 18, 2020
Merged

REST XML Request Functionality#123
mattBrzezinski merged 7 commits into
v1from
MB/rest-xml-service

Conversation

@mattBrzezinski

@mattBrzezinski mattBrzezinski commented Jun 9, 2020

Copy link
Copy Markdown
Member

TODO

I wanted to get this merge request up for review ASAP, there are some things which I know need to be addressed and can be omitted from being review:

  • Compat entries for MbedTLS, Sockets, XMLDict
  • Additional testing for src/AWS.jl functionality; _sign2(), _sign4(), etc.
  • Properly typing AWSException.info

Overview

Note this merge request is targeting the v1 branch, and does not need a version bump or anything of the sorts. This merge request introduces the re-writing for REST XML requests as part of the re-write. It also includes a re-write AWSConfig as a struct proposed by @ararslan here.

I think the best way to review this merge request is to look at the commits themselves and not the files changed. The majority of the changes are to auto-generated files such as src/AWSServices.jl and src/services/*.jl.

Next Steps

The re-write of AWS.jl is almost complete, the last few pieces of the puzzle are to:

  1. Implement the other request functionality: JSON, Query, and Rest JSON
  2. Remove all usages of AWSCore.jl and use the appropriately re-written functions
  3. Setup the GitHub action to run the auto-generation of code
  4. Setup bors, and CI, house keeping things

@ararslan

ararslan commented Jun 9, 2020

Copy link
Copy Markdown
Member

It also includes AWSConfig as proposed by @ararslan here.

I haven't been following the development of this package, but note that if AWSConfig is used in the same way it currently is in AWSCore et al., this implementation won't work; there seems to be quite a bit of existing code that assumes the AWSConfig object is a mutable AbstractDict and can store arbitrary data. Just something to keep in mind in case you weren't already cognizant of that.

@mattBrzezinski

Copy link
Copy Markdown
Member Author

It also includes AWSConfig as proposed by @ararslan here.

I haven't been following the development of this package, but note that if AWSConfig is used in the same way it currently is in AWSCore et al., this implementation won't work; there seems to be quite a bit of existing code that assumes the AWSConfig object is a mutable AbstractDict and can store arbitrary data. Just something to keep in mind in case you weren't already cognizant of that.

It's not a direct port from what you had previously proposed. I just made AWSConfig it's only mutable struct and changed places which had used the AbstractDict interface to access it's fields. Updating to clarify that!

Comment thread src/AWSConfig.jl Outdated
Comment thread src/AWSConfig.jl Outdated
Comment thread src/AWSConfig.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl
Comment thread src/AWS.jl
Comment thread src/AWSExceptions.jl Outdated
@mattBrzezinski
mattBrzezinski force-pushed the MB/rest-xml-service branch 2 times, most recently from 482956c to ed8d4f8 Compare June 17, 2020 05:30
Comment thread test/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl Outdated
Comment thread src/AWS.jl
Comment thread test/patch.jl Outdated
- Mock AWS._http_request()
- Minor string performance improvements
- Formatting styles to align with BlueStyle
- AWS sign functions return the request they are modifying
- Service URL generation takes in region::String rather than
  config::AWSConfig
- Added return_headers as kwarg for AWS Requests
- Added testing for src/AWS.jl functionality
@mattBrzezinski
mattBrzezinski merged commit 83a68f7 into v1 Jun 18, 2020
@mattBrzezinski
mattBrzezinski deleted the MB/rest-xml-service branch June 19, 2020 03:12
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