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

Project Status #35

Closed
gtrak opened this issue May 8, 2018 · 22 comments
Closed

Project Status #35

gtrak opened this issue May 8, 2018 · 22 comments

Comments

@gtrak
Copy link
Contributor

gtrak commented May 8, 2018

I'm considering trying to spend my own time to get this library to a more polished state for our own company's use. It seems like a few PR's have languished, and I'm wondering about LOE to get it more feature-complete. Is there anything specific blocking work here or is it just a lot of details?

@seliopou
Copy link
Member

seliopou commented May 14, 2018

Hey @gtrak, thanks for your interest in the library and your questions. There are a few things that are blocking expanding the number of supported services and development in general.

First is that doing releases right now is a huge, manual pain. Once the library switches to using jbuilder, each service library can just use the GitHub release tarball, rather than library-specific ones generated by some hokey scripts I wrote. I have a milestone for that.

Second is support for a "map" data structure in the core library that code generation can target. Something like that is included in #33, but that PR includes a bunch of changes that haven't been separated out into PRs that are easy to review. It also doesn't include generated code. Including that in separate PR and getting it merged should make it possible to do...

Third, support all service APIs. For some, the existing code generation will suffice. For others, it'll be necessary to implement different protocols.

That's what the project needs in order to get going again. I believe I gave @tmcgilchrist repo access to review and merge contributions as well. If you have any other questions let me know!

@gtrak
Copy link
Contributor Author

gtrak commented May 14, 2018

Thanks! I spent yesterday trying to understand the existing Oasis build and seeing if I can translate it to jbuilder. I managed to gen code for s3 and compile it with jbuilder. I will keep going down that path, it seems to have the least overlap with existing PRs.

@anmonteiro
Copy link
Contributor

anmonteiro commented May 14, 2018

@gtrak I’d be interested to see your generated code for S3.
When I last looked at it I made it work but it required very many changes to the code (even without multipart uploads).

@seliopou I’d be happy to split #33 in smaller PRs as you see fit. I also didn’t commit the generated code because it would just make the diff even messier. Would love to get it in and make progress as you see fit. Let’s not make all that work be for nothing

@gtrak
Copy link
Contributor Author

gtrak commented May 14, 2018

@anmonteiro I actually started working from your more-apis branch. I haven't proven out the calls themselves, but the generated code does compile. I had to switch to ppx_tools.versioned for the sake of metaquot, and added a List.find_opt helper for 4.04.2 to be able to compile it, plus fixed a few deprecation warnings.

Next up for me is generating jbuild files instead of oasis and figuring out packaging.

@gtrak
Copy link
Contributor Author

gtrak commented May 14, 2018

@anmonteiro gtrak@2d311de

@gtrak
Copy link
Contributor Author

gtrak commented May 14, 2018

This one includes the jbuilds: gtrak@97b7252

@tmcgilchrist
Copy link
Collaborator

Thanks for your interest @gtrak feel free to copy me in on any changes/ideas/prs.

Luckily I'm back doing OCaml at work so I have more headspace to work on OCaml libraries.
I made a start on building things with jbuilder/dune last year https://github.com/tmcgilchrist/ocaml-aws/tree/topic/jbuilder. That code uses jbuilder to build the top level projects of

  • src/aws_gen - generates the other was libraries based of input/ botocore definitions
  • lib/aws_core - common code across aws_* generated libraries

The generator aws_gen still outputs oasis based builds (this should be changed to jbuilder see src/templates.ml.

After that there are 3 remaining issues:

  1. Generator bugs around duplicate cases in match statements
  2. Generator bug where some types aren't exposed or imported correctly in the generated modules. This causes build failures.
  3. More tests :-)

It would also be nice to have better automation of building and releasing updates to the library as a whole but that follows on from the above work.

@gtrak
Copy link
Contributor Author

gtrak commented May 28, 2018

@tmcgilchrist I'm going to take a look at your branch today and see if I can pick off any of those issues.

@gtrak
Copy link
Contributor Author

gtrak commented May 29, 2018

I think I've addressed 1. and 2. in tmcgilchrist#2 . It seems like you already did the work to output jbuilds instead of oasis? I'm going to look into generating s3 bindings again.

@anmonteiro
Copy link
Contributor

@gtrak my branch from #33 generates working S3 bindings

@anmonteiro
Copy link
Contributor

@seliopou I'm happy to split my work from #33 into separate parts, as well as including the generated code, but I need some direction as to what those parts should be. Thoughts?

@gtrak
Copy link
Contributor Author

gtrak commented May 29, 2018

@anmonteiro I started working off @tmcgilchrist's branch since it seemed the most active, not sure if that was the right choice, or if we want to merge all that stuff before #33 can land, but the s3 work looks self-contained to me.

@anmonteiro
Copy link
Contributor

@gtrak I'm not sure it is that self-contained. AFAIK most work I had to do in my branch was to get S3 working, but it's been a while

@seliopou
Copy link
Member

@anmonteiro Looks like your first commit in your PR is self-contained. There's some code in some other commit that computes endpoints for all the services. Those could be separate PRs.

The repository to my knowledge has always maintained the invariant that running code generation won't change the generated code for the services. That should be maintained.
When I have time I'll add a test to checksum the generated libraries, run code generation, and checksum again to ensure this. For now, I've just been including the code generation step as the last commit of the PR that makes the change.

@seliopou
Copy link
Member

Ah, the >= 4.02 commit's already been merged. My mistake.

@gtrak
Copy link
Contributor Author

gtrak commented May 31, 2018

@anmonteiro I cherry-picked your 'succesfully generate s3' commit over the latest, and I'm not seeing generated types.ml or errors.ml for s3. Did you end up fixing that somewhere?

At least for the errors, I've found the comment: (* there might be a weird bug here that is not generating the errors *) and am trying to understand those cases.

@gtrak
Copy link
Contributor Author

gtrak commented May 31, 2018

@anmonteiro Also I'm looking through your PR and I don't see a generated s3 directory. Which features from that PR are actually required to get S3 working, and which are general issues that have overlap with the jbuilder branch? For example, I added empty structs support with a unit type, similarly to how you did it, but that was for the sake of an RDS codegen issue. I would like to avoid solving the same problems you already have.

I think the build system work should take precedence in merge order over #33, even though your work was earlier. The repo is currently a moving target, but we need 4.03+, jbuilder support and releases as a baseline to make future commits smaller. I hope you're ok with my attempt at pulling out what small chunks I can from #33 :-).

@anmonteiro
Copy link
Contributor

@seliopou I'll rebase my commits interactively into separate, logical commits (if I can remember exactly which are logical separate parts – it's been a while).

I'll also include 2 other things: 1) the updated botocore definitions, which I was using to develop #33, and the generated code.

@gtrak the "successfully generate code for s3" only does exactly that, all the other commits are still necessary to successfully invoke the service.

Is the jbuilder PR ready? I strongly suggest we don't duplicate work, but instead try to join our efforts in at least salvaging part of what #33 proposes. I'm more than happy to rebase on top of the build system changes.

@gtrak
Copy link
Contributor Author

gtrak commented May 31, 2018

It seems like the jbuilder branch is close to ready here: tmcgilchrist#1 . I guess I'm offering to help with the rebase, but feel free. @tmcgilchrist was supportive of me PR'ing directly to that branch.

@tmcgilchrist
Copy link
Collaborator

I feel like I am pretty close with tmcgilchrist#1
The outstanding bits are cleaning up the Makefile and scripts to generate releases, maybe some documentation and some real testing of the various core services people care about (S3,EC2 RDS?)

I'd be happy with a PR against my branch that added working S3. Today I have some time to work on this and will try pulling any S3 changes across. I would really appreciate any tests, no matter how trivial for the various services.

@tmcgilchrist
Copy link
Collaborator

Status update for beginning of 2019.

I'm almost at the point where I have the services supported that I personally use / care about, need to finish off support for EMR, DynamoDb and ECS.

More work needs to be done on the testing side of things, as part of the generation step I have been adding an empty test suite for each service. Ideally there would be some basic examples ported from the official AWS documentation and put into the test suite #37. Following that having proper CI that runs the tests against AWS, rather than manually having to run them would be great #49.

Further enhancements that would be great:

@tmcgilchrist
Copy link
Collaborator

Version 1.2 released to opam https://opam.ocaml.org/packages/aws/
Closing in favour of more focused issues.

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

No branches or pull requests

4 participants