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

feat: test reusable workflow in e2e test #136

Merged
merged 5 commits into from
Jun 30, 2023
Merged

Conversation

sailorlqh
Copy link
Contributor

fix: #128

@sailorlqh sailorlqh marked this pull request as ready for review June 27, 2023 17:36
@sailorlqh sailorlqh requested a review from a team as a code owner June 27, 2023 17:36
@sailorlqh sailorlqh requested review from drevell, yolocs, sqin2019, capri-xiyue, verbanicm and bradegler and removed request for yolocs and drevell June 27, 2023 17:36
@sailorlqh
Copy link
Contributor Author

sailorlqh commented Jun 27, 2023

Added @bradegler and @verbanicm as reviewers to get some input on the terraform code as they are the experts.

@sqin2019
Copy link
Contributor

sqin2019 commented Jun 27, 2023

Please correct me if I am wrong, you want to test both reusable workflows (upload file to gcs via reusable workflow) and code logic (manually upload file to gcs) on whatever code changes. It seems redundant. We can test reusable workflow when there are changes to them, if no changes to these workflow files then test the code logic. Therefore, we can have a separate workflow to test reusable workflow. Also smaller PRs are preferred.

Another way is to test reusable workflow -> only check if files successfully uploaded to gcs in ci, test the entire e2e in dev when doing the release, this way we don't need to change the current ci infrastructure. Only concern is what if e2e fail, we need to fix it asap and release a new version.

Just my thoughts, I will wait for other people's review.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@sailorlqh
Copy link
Contributor Author

sailorlqh commented Jun 27, 2023

Please correct me if I am wrong, you want to test both reusable workflows (upload file to gcs via reusable workflow) and code logic (manually upload file to gcs) on whatever code changes. It seems redundant. We can test reusable workflow when there are changes to them, if no changes to these workflow files then test the code logic. Therefore, we can have a separate workflow to test reusable workflow.

The idea for this update is to ensure the workflow's behavior is correct. According to previous discuss with Chen, we decide to add this to the CI e2e test.

Also smaller PRs are preferred.

I don't think I can further break down this PR, as calling a reusable workflow in ci.yml will require the related terraform code change, and also the tests should be updated.

Another way is to test reusable workflow -> only check if files successfully uploaded to gcs in ci, test the entire e2e in dev when doing the release, this way we don't need to change the current ci infrastructure. Only concern is what if e2e fail, we need to fix it asap and release a new version.
To make sure the reusable workflow can upload files to GCS, we still need to call the workflow, thus the terraform code change is inevitable. And we didn't make any ci infra change, we just divided them into 3 different jobs.
Testing in dev will require us to update the cloud run images if we made modification in the code, and I don't think doing a new release every time we makes modification to workflow files is the optimal way.

The current way we can test all the things in ci project, and the current set up can ensure the env are cleaned up after test is running, these are the reason I implemented this way.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@sailorlqh sailorlqh force-pushed the sailorlqh/e2e_test_wf branch 2 times, most recently from e87780b to 989deae Compare June 28, 2023 22:26
@sailorlqh sailorlqh requested a review from bradegler June 29, 2023 00:35
bradegler
bradegler previously approved these changes Jun 30, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
test/integration/main_test.go Show resolved Hide resolved
test/integration/main_test.go Show resolved Hide resolved
test/integration/main_test.go Show resolved Hide resolved
test/integration/main_test.go Show resolved Hide resolved
test/integration/main_test.go Show resolved Hide resolved
capri-xiyue
capri-xiyue previously approved these changes Jun 30, 2023
@sailorlqh sailorlqh merged commit 3aa7f54 into main Jun 30, 2023
@sailorlqh sailorlqh deleted the sailorlqh/e2e_test_wf branch June 30, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

validate workflow result in integ test.
4 participants