copilot-cli icon indicating copy to clipboard operation
copilot-cli copied to clipboard

chore: Docker Compose import milestone 1 - library setup and basic service conversion

Open rclinard-amzn opened this issue 3 years ago • 4 comments

Note: This is targeting the experiment/docker-compose-import branch. It is not targeting the mainline release branch. Also note that most of the added code is unit tests - there's <250 lines of actual functional code.

This PR implements the first milestone of the Docker Compose import experiment. We set up the library and handle the Compose concepts that mostly mirror 1:1 with Copilot fields.

In the next PR, I will add detection for unsupported Compose fields that this experiment won't support.

Relates to https://github.com/aws/copilot-cli/issues/1612

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

rclinard-amzn avatar Aug 11 '22 19:08 rclinard-amzn

One issue that needs to be addressed after this PR - how do we handle LBWS conversion without duplicating tons of code? The LBWS manifest is almost exactly the same as the Backend Service manifest, but they use two different data structures in our codebase because the Port field is optional on Backend services. Alternatively, we can just not add the same level of unit tests for the LBWS conversion function since the logic should otherwise be the same.

rclinard-amzn avatar Aug 11 '22 19:08 rclinard-amzn

One issue that needs to be addressed after this PR - how do we handle LBWS conversion without duplicating tons of code? The LBWS manifest is almost exactly the same as the Backend Service manifest, but they use two different data structures in our codebase because the Port field is optional on Backend services. Alternatively, we can just not add the same level of unit tests for the LBWS conversion function since the logic should otherwise be the same.

Unit tests are not good to skip even in these cases because they're partly needed to alert people when things break. Because they may be similar code it may be attractive to skip writing them, but they can save you lots of time debugging, and help you find bugs before they're sent to production. It's not just to test the logic of code, but also to make sure that the code is what you think it is, typos can break a surprising amount of stuff 😄

CaptainCarpensir avatar Aug 12 '22 16:08 CaptainCarpensir

That's a good point! I think that once the Backend Service handling is fleshed out in a PR or two, the LBWS handling can be copy pasted from that, instead of having to copy paste every time I make a logic change to Backend Service handling. That should amortize the cost of copy-pasting and avoid introducing minor deviations along the way!

rclinard-amzn avatar Aug 12 '22 16:08 rclinard-amzn

One issue that needs to be addressed after this PR - how do we handle LBWS conversion without duplicating tons of code? The LBWS manifest is almost exactly the same as the Backend Service manifest, but they use two different data structures in our codebase because the Port field is optional on Backend services. Alternatively, we can just not add the same level of unit tests for the LBWS conversion function since the logic should otherwise be the same.

We do plan to refactor that package to reduce the duplicated code. One day...!

huanjani avatar Aug 12 '22 16:08 huanjani

Thanks for the great feedback, that should all be addressed now!

rclinard-amzn avatar Aug 16 '22 00:08 rclinard-amzn