Skip to content
This repository has been archived by the owner on Mar 7, 2024. It is now read-only.

Add logpush param #90

Merged
merged 3 commits into from Jan 17, 2023
Merged

Add logpush param #90

merged 3 commits into from Jan 17, 2023

Conversation

myakhnis-shopify
Copy link

@myakhnis-shopify myakhnis-shopify commented Jan 10, 2023

Description

  • Adds logpush support based on https://github.com/cloudflare/cloudflare-go/pull/1160 (but adapted to work with older APIs)

Has your change been tested?

Tested internally

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.

@sbfaulkner
Copy link

fwiw I think this is already upstream ... we're trying to figure out the best way to get back in sync or rebase with upstream since some of the most recent upstream changes have made rebasing more difficult than the last time(s)

@myakhnis-shopify
Copy link
Author

Yeah, I based my change partially off of theirs. But it looks like they refactored some of workers.go so we'll probably have to manually review/reapply changes whenever we want to do that.

@sbfaulkner
Copy link

@myakhnis-shopify
yeah... I'm looking at that now because we need the upstream changes for the terraform provider :-/

@sbfaulkner
Copy link

sbfaulkner commented Jan 13, 2023

as per slack DM...

  • we (assets/traffic/prodeng) no longer need a fork of the terraform provider, which means we no longer need a fork of cloudflare-go
  • the upstream changes are not compatible with the current local changes

for reference the list of PRs that are included in the local branch...

most (if not all) of these likely become unneeded once Workers for Platforms is adopted

My suggestion is to disconnect the fork and rename this repo for oxygen-specific "legacy" usage, so that if/when we need to fork the official master again we can.

@myakhnis-shopify
Copy link
Author

My suggestion is to disconnect the fork and rename this repo for oxygen-specific "legacy" usage

I chatted with the team, and we agree it's reasonable. That being said, this change will likely still be merged, and I'll file a separate ticket for the rename.

@myakhnis-shopify myakhnis-shopify marked this pull request as ready for review January 17, 2023 16:11
Copy link

@uri uri left a comment

Choose a reason for hiding this comment

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

Checked against upstream looks good.

@myakhnis-shopify myakhnis-shopify merged commit efac4b0 into master Jan 17, 2023
@myakhnis-shopify myakhnis-shopify deleted the add-logpush-param branch January 17, 2023 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants