ci: add cdk e2e tests

[vsiravar]

Issue #, if available:

Description of changes:
Run cdk integration tests with finch as the drop in replacement for docker in cdk, i.e CDK_DOCKER=finch

Testing done:
Yes in fork, see https://github.com/vsiravar/finch-core-public/actions/runs/6126870804/job/16631723973

• I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ningziwen]

Any reason adding to finch-core instead of finch?

[vsiravar]

Any reason adding to finch-core instead of finch?

Mainly to reduce noise in finch and use an intermediate repo like finch-core which have very under utilized runners.

[sam-berning]

Any reason adding to finch-core instead of finch?

Mainly to reduce noise in finch and use an intermediate repo like finch-core which have very under utilized runners.

It's a good goal, but I think these tests still fit better in finch. We're testing against finch, so the workflow can be affected by changes in finch (which we wouldn't expect in finch-core). It'd be too easy to merge a change in finch that breaks these e2e tests, then not check finch-core, and inadvertently roll out a breaking change.

Assuming the tests don't take a super long time to run, my suggestion would be to put them in finch and run them on every PR and merge.

[sam-berning]

why is this a schedule rule instead of running on PRs and merges? do the tests take too long? it feels like this workflow would be too easy to ignore

[vsiravar]

The reason why it sits in finch-core and also why it does not run against every PR is that we don't want to block finch PRs if cdk adds a feature to it's main branch which is not yet compatible with finch. finch will try to bridge the gap asap but cdk will also not block it's release if finch is not compatible with some container constructs. I do agree that it's easy to ignore this workflow. How about adding an action in this workflow to open an issue if the test fails, would that help get better visibility when there is incompatibility?

Ideally this should sit in the cdk repo IMO since finch is a dependency for cdk and not the other way around.

[sam-berning]

maybe the right move would be to pin cdk & tests to a specific version/commit? that way we can isolate finch regressions.

How about adding an action in this workflow to open an issue if the test fails, would that help get better visibility when there is incompatibility?

yeah, that's a good idea

[vsiravar]

maybe the right move would be to pin cdk & tests to a specific version/commit? that way we can isolate finch regressions.

That's an interesting idea to catch regressions in finch but not for cdk. If cdk adds an integration test in their suite or adds another construct that uses docker/finch, we won't be testing that if we pin the commit.

How about adding an action in this workflow to open an issue if the test fails, would that help get better visibility when there is incompatibility?

yeah, that's a good idea

Cool, I'll add an action to open issues if the test suite fails.

[sam-berning]

maybe the right move would be to pin cdk & tests to a specific version/commit? that way we can isolate finch regressions.

That's an interesting idea to catch regressions in finch but not for cdk. If cdk adds an integration test in their suite or adds another construct that uses docker/finch, we won't be testing that if we pin the commit.

we can always bump cdk to a new version/commit. it'd be a pain to do that manually all the time, but maybe we could add a schedule action to open a new PR to bump cdk to a new version and we can test it while merging the PR?

[vsiravar]

Updated to pin the sha. I am going to create another PR to open an issue if the run fails based on this action: https://github.com/marketplace/actions/create-an-issue . Having trouble with the token atm but should be easy to fix.

[sam-berning]

I don't think it makes sense to pin the SHA if we're sticking with the scheduled approach -- that suggestion was meant for a workflow that runs on every Finch PR and blocks merges. Under that approach, limiting the tests to only the Finch changes is helpful because we don't want development on Finch to be blocked by an unrelated change to the CDK.

If we're going with the schedule approach, since it doesn't block Finch PRs, I think it makes sense to always run it against the latest version of CDK to discover issues that changes to the CDK might introduce.

[vsiravar]

Reverted the change back to track main branch.

[vsiravar]

Merging this since #203 (comment) is a known issue.