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

LogpushFilter support #915

Merged
merged 7 commits into from May 31, 2022
Merged

Conversation

yknx4
Copy link
Contributor

@yknx4 yknx4 commented May 30, 2022

Adds support for filter field in LogpushJob as set in here

Description

  • Added new structs for Filters
  • Added Operator enums for valid operators
  • Added helper function to attach filter (since it is a json string and not a json object)
  • Added a validation to ensure filters are sound

Has your change been tested?

Added struct test that matches example in Cloudflare docs

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

@yknx4 yknx4 requested a review from jacobbednarz as a code owner May 30, 2022 20:02
@jacobbednarz
Copy link
Member

this PR looks great, thank you! my only hesitation at the moment is the job.AddFilter call. within the rest of the library, we put the filtering parameters directly on the struct itself without a need for the additional method. example:

job := cloudflare.LogpushJob{
	Name:            "example.com static assets",
	LogpullOptions:  "fields=RayID,ClientIP,EdgeStartTimestamp&timestamps=rfc3339&CVE-2021-44228=true",
	Dataset:         "http_requests",
	DestinationConf: "s3://<BUCKET_PATH>?region=us-west-2/",
	Filter: cloudflare.LogpushJobFilters{
		Where: cloudflare.LogpushJobFilter{
			And: []cloudflare.LogpushJobFilter{
				{Key: "ClientRequestPath", Operator: cloudflare.Contains, Value: "/static\\"},
				{Key: "ClientRequestHost", Operator: cloudflare.Equal, Value: "example.com"},
			},
		},
	}
}
    

however, taking this approach, we'll need a custom marshal handler which may be annoying. WDYT?

@yknx4
Copy link
Contributor Author

yknx4 commented May 30, 2022

@jacobbednarz It makes a lot of sense to keep the consistency of the library. Let me do some refactoring to add custom marshalling for that field and an extra test to ensure bot Marshalling and Unmarshalling works as expected

@jacobbednarz
Copy link
Member

awesome, thanks! other than that, the testing and approach looks ace.

@yknx4
Copy link
Contributor Author

yknx4 commented May 30, 2022

@jacobbednarz Refactoring done ~

logpush.go Outdated Show resolved Hide resolved
logpush_test.go Outdated Show resolved Hide resolved
logpush.go Outdated Show resolved Hide resolved
yknx4 and others added 3 commits May 30, 2022 17:23
Co-authored-by: Jacob Bednarz <jacob.bednarz@hey.com>
Co-authored-by: Jacob Bednarz <jacob.bednarz@hey.com>
Co-authored-by: Jacob Bednarz <jacob.bednarz@hey.com>
@jacobbednarz jacobbednarz merged commit 36002e2 into cloudflare:master May 31, 2022
@jacobbednarz
Copy link
Member

thank you for the contribution, you rock! 🥇

@yknx4 yknx4 deleted the logpushfilter-support branch May 31, 2022 02:11
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.

None yet

2 participants