search

developmentseed / eoapi-cdk

AWS CDK constructs for deploying eoAPI
https://developmentseed.org/eoapi-cdk
11 stars 4 forks source link

feat: add integration tests #69

Closed vincentsarago closed 8 months ago

vincentsarago commented 1 year ago

closes #13

second take at #67

emileten commented 1 year ago

Anybody would like to take another look ? The action is running fine (I ran them manually to remove any bug). Here is a gist of what's happening :

It takes a while, almost an hour from scratch :

For another time I'd like to figure out how to cache all the lambda runtime builds but it's a rabbit hole....

emileten commented 1 year ago

@vincentsarago @sharkinsspatial, following-up on a discussion I had with Vincent on Slack.

There is a concern regarding the fact that we are going to run the deploy and test workflow using the eoapi-template IaC repo.

What's the concern ? Let's assume that I submit a PR to eoapi-cdk with a breaking change, and I don't correspondingly modify the eoapi-template repo IaC. If I merge this PR, a release happens, which triggers a deployment using the new release wrapped into the non-updated eoapi-template code. This deployment is going to fail. It would not have failed, had I correspondingly updated eoapi-template.

This means that for any PR that triggers a release, we need to make sure we update eoapi-template if needed. This 'makes sense', because in this framework, eoapi-template became part of the test suite, and any test suite must be updated if breaking changes are merged.

Pros

  1. eoapi-template, which was meant to be an official example, is tested and up to date.
  2. we don't have to write wrapper IaC just for the purpose of tests, instead we reuse the official example. I am worried about writing IaC just for the tests and ending up with a lot of IaC to manage (eoapi-template, test IaC, the constructs...).

Cons

  1. eoapi-template has to be modified often.

To this, I answer "yes but not that much often, because we have to update it only for breaking change releases. I would also answer that we do want to update eoapi-template often anyways because it's the official example"

  1. It's hard to remember to update it because it's in a separate repo.

To this, I answer "we'd have to update any IaC we write for the purposes of the deploy-test workflow, whether it's eoAPI template or not. If the difficulty is that it's hard to remember because it's in a separate repo, I suggest that we move eoapi-template to be within eoapi-cdk."

vincentsarago commented 1 year ago
  1. eoapi-template has to be modified often.

To this, I answer "yes but not that much often, because we have to update it only for breaking change releases. I would also answer that we do want to update eoapi-template often anyways because it's the official example"

eoapi-template will have to be modified on every release because we are using direct version pinning in eoapi-template (https://github.com/developmentseed/eoapi-template/blob/main/requirements.txt#L3)

I personally having circular dependency with external repo is dangerous is will required more work (e.g if someone change something in eoapi-cdk but has no idea what is eoapi-template is, they might open a PR and wait for someone to review, which mean that the reviewer might have to review two PR in two different repo.

I do agree that eoapi-template should be the official demo application but I do not agree that we have to keep 1:1 with the versions.

I suggest that we move eoapi-template to be within eoapi-cdk."

Because the purpose of eoapi-template is to be a repository template, I think we should could it separate. IMO it will be fine to just copy/paste what application we have in eoapi-cdk test suite to eoapi-template

emileten commented 1 year ago

Because the purpose of eoapi-template is to be a repository template, I think we should could it separate. IMO it will be > fine to just copy/paste what application we have in eoapi-cdk test suite to eoapi-template

👍

emileten commented 1 year ago

@vincentsarago made the changes, the tests here use their own wrapper CDK code. Looks like I adressed your main requests, anything else ? Once the action succeeded I will turn off the on: push and leave only the "on-release" trigger and manual option.

vincentsarago commented 1 year ago

Thanks @emileten I'm wondering what's the purpose of doing integration test after pushing packages on npm and pypi. IMO we should do integration tests and then publish.

sharkinsspatial commented 1 year ago

@emileten I concur with @vincentsarago here and I like all the changes made to have the local integration test app self-contained within the repo.

After this PR is merged, we should consider deprecating eoapi-template and point the documentation to the integration test app stack to avoid duplication and needing to maintain both.

emileten commented 1 year ago

Sounds good @sharkinsspatial

vincentsarago commented 11 months ago

@emileten is there any specific thing to do here?

emileten commented 11 months ago

@emileten is there any specific thing to do here?

@vincentsarago I think it's almost good to merge but I wanted to get this one merged first https://github.com/developmentseed/eoAPI/pull/144

vincentsarago commented 9 months ago

@emileten any chance we could get this PR merged?

vincentsarago commented 8 months ago

@emileten can we merge this? we're going to clean up a bit all the eoapi-* repo next week (mostly to make eoapi-template an important piece for onboarding new users)

emileten commented 8 months ago

@vincentsarago sorry I let this become stale. Resuming work on this one today !

emileten commented 8 months ago

Still in progress ; I am having issues finding the builds in the local filesystem of the github runner, to deploy : https://github.com/developmentseed/eoapi-cdk/actions/runs/7962946517/job/21737522652.

emileten commented 8 months ago

Making progress on this, now I am trying to figure out why I get a TimeOutError from httpx in the bootstrapper.

Because this bootstrapper runs fine on my local machine, my guess is that the bootstrapper lambda doesn't have access to the internet because of accidental configuration changes I made ; in particular, I removed allowPublicSubnet: true from the lambda creation parameters, as AWS explicitly says the lambda does not have access to the internet with this parameter.

Doesn't seem to be solving the problem, though.

Edit : oh I know what's going on - I was placing things in private subnets. I'll simplify the setup in the test to mimic https://github.com/developmentseed/eoAPI/tree/main/infrastructure/aws/cdk.