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

Add CI tests #10

Merged
merged 12 commits into from
Nov 21, 2023
Merged

Add CI tests #10

merged 12 commits into from
Nov 21, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Nov 14, 2023

Will it test? That is the question.

Essentially copying the test CI and code from deck after Kong/deck#1109. Will need to figure out what to snip out later.

@GGabriele
Copy link
Collaborator

@rainest assuming (also based on the commits) this PR is basically this other PR plus tests, can we use chore/lift-and-shift as base branch for this PR in order to help with diffs?

@rainest rainest changed the base branch from main to chore/lift-and-shift November 14, 2023 21:21
@rainest
Copy link
Contributor Author

rainest commented Nov 14, 2023

Sure.

@rainest rainest mentioned this pull request Nov 15, 2023
.ci/check.sh Outdated Show resolved Hide resolved
.ci/setup_kong.sh Outdated Show resolved Hide resolved
"github.com/fatih/color"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/kong/deck/cmd"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm as noted in the linked issue this might not be breaking the build but is rather undesirable 🤔

Couldn't we leave whatever's using the cmd in deck repo and test here without resorting to deck commands? This way we'd break up the dependency. Not sure how feasible that is given that most of the code here relies on issuing requests against Kong (or Konnect) via the cmd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As designed the integration tests are very much designed around using the CLI commands as they would be used at the CLI. We could build a pseudo-CLI to call the various package functions in sequence here, but in practice that code would be almost identical to the CLI minus cobra and such. IDK how much we'd need the separation in form if not function to meet the goals of the separation--that's more a question for @Tieske probably.

My hope is that after the initial separation we do continue with further refactors to make the packages and their APIs here better designed as modular components, and in the course of those refactors we write new integration tests that slowly replace the original deck ones.

Is there, alternately, a way to automatically run an external repo's tests against a PR branch as part of that PR's checks? We have good reason to run deck and KIC tests against proposed library changes regardless. Being able to do that without actually importing their packages here feels less backwards, for lack of a better word.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to run deck and/or KIC tests with changes in this repo then simply checking those out via something like https://github.com/Kong/kubernetes-ingress-controller/blob/44ef693b3cae49a55475cde4097af28739f36918/.github/workflows/release_docs.yaml#L26-L31 and then:

go mod edit -replace=github.com/kong/go-database-reconciler=../go-database-reconciler
go mod tidy

(assuming checkout to the same parent dir)


As for the refactors: 👍. It would be highly desirable to remove that (semi-)circular dependency between this repo and deck.

We'd basically need to peel off one layer (cmd invocation) and use the actual library code (but that's just my speculation). deck tests should be left as is anyway to utilize this lib in its integration suite.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rainest rainest requested a review from pmalek November 16, 2023 19:59
@rainest rainest force-pushed the chore/test branch 4 times, most recently from da02ed2 to 45deed3 Compare November 16, 2023 20:58
@rainest
Copy link
Contributor Author

rainest commented Nov 16, 2023

Re-added Codecov now that I've realized this doesn't require an org admin to configure and added a token.

In deck (and KIC) we weirdly have the CODECOV_TOKEN in a configure CI environment, but we don't use this environment in the workflow configuration (which apparently doesn't cause any issues). I'm not sure what's up with that. I've just placed it in the main repo secrets for now.

Copy link
Collaborator

@GGabriele GGabriele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor cleanup comments

.github/workflows/integration.yaml Show resolved Hide resolved
.github/workflows/integration.yaml Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Base automatically changed from chore/lift-and-shift to main November 17, 2023 20:35
@rainest rainest marked this pull request as ready for review November 21, 2023 16:04
@rainest rainest requested review from a team and GGabriele November 21, 2023 16:04
@rainest rainest merged commit c5e943e into main Nov 21, 2023
15 of 16 checks passed
@rainest rainest deleted the chore/test branch November 21, 2023 19:07
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.

4 participants