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(integrations/cloudfilter): introduce behavior tests #4973

Merged
merged 15 commits into from
Aug 13, 2024

Conversation

ho-229
Copy link
Contributor

@ho-229 ho-229 commented Aug 6, 2024

What changes are included in this PR?

  • Behavior tests for integration/cloudfilter.
  • Behavior tests workflow.
  • Bump cloud-filter to 0.0.5.

@ho-229
Copy link
Contributor Author

ho-229 commented Aug 11, 2024

Hi @Xuanwo and @oowl , please take a look.

@@ -270,6 +286,20 @@ def generate_bin_cases(

return cases

def generate_integration_cases(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to touch here if cloudfilter only support fs now?

connect-token: ${{ secrets.OP_CONNECT_TOKEN }}

- name: Test Integration CloudFilter
uses: ./.github/actions/test_behavior_integration_cloudfilter
Copy link
Member

Choose a reason for hiding this comment

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

We can test directly here without altering the entire integration test system if Cloudflitler only supports fs.

@ho-229
Copy link
Contributor Author

ho-229 commented Aug 11, 2024

Fixed cc @Xuanwo

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I want to minimize the changes to the entire codebase before the cloud filter is ready for production use.

Would you like to revert changes to the integration test system?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can also remove those changes.

Copy link
Member

Choose a reason for hiding this comment

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

The same.

@@ -145,3 +145,17 @@ jobs:
with:
os: ${{ matrix.os }}
cases: ${{ toJson(matrix.cases) }}

test_integration_cloudfilter:
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this anymore?

name: Behavior Test Integration CloudFilter

on:
workflow_call:
Copy link
Member

Choose a reason for hiding this comment

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

We can use on pull_request instead.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I want to minimize the changes to the entire codebase before the cloud filter is ready for production use.

Would you like to revert changes to the integration test system?

@ho-229
Copy link
Contributor Author

ho-229 commented Aug 13, 2024

cc @Xuanwo , PTAL

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Other LGTM, thanks a lot for your work!

Comment on lines 47 to 55
# TODO: 1password is only supported on linux
#
# Waiting for https://github.com/1Password/load-secrets-action/issues/46
- name: Setup 1Password Connect
if: runner.os == 'Linux'
uses: 1password/load-secrets-action/configure@v1
with:
connect-host: ${{ secrets.OP_CONNECT_HOST }}
connect-token: ${{ secrets.OP_CONNECT_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this.

with:
need-protoc: true
need-rocksdb: true
github-token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this.

@ho-229
Copy link
Contributor Author

ho-229 commented Aug 13, 2024

Fixed @Xuanwo

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you!

@Xuanwo Xuanwo merged commit 890dd28 into apache:main Aug 13, 2024
167 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants