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

Adding Page Shield support #1459

Merged
merged 9 commits into from
Dec 7, 2023
Merged

Conversation

wiremanb
Copy link
Contributor

@wiremanb wiremanb commented Dec 6, 2023

Description

Added support for Page Shield based on developer docs.

Has your change been tested?

All changes have been tested with test suites. Also, have tested using a local driver application

Screenshots (if appropriate):

Screenshot 2023-12-05 at 7 05 17 PM

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 in cloudflare/api-schemas
    and relies on stable APIs.

Copy link
Contributor

github-actions bot commented Dec 6, 2023

changelog detected ✅

@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 6, 2023

@jacobbednarz do you think it is necessary to use ptr's for boolean values in this? How stringent do you want me to be with the linter issues?

@jacobbednarz
Copy link
Member

the semgrep rules are in place from past issues we've seen with usage (especially the terraform provider). i'm not sure why but semgrep should be pointing you at https://github.com/cloudflare/cloudflare-go/blob/master/docs/conventions.md#booleans which explains it a little more too.

@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 6, 2023

I noticed some other files that don't use it, that's why I ask. I will update and push another. Apologies for the repetitive nature.

@jacobbednarz
Copy link
Member

i've gone through and applied a few updates from https://github.com/cloudflare/cloudflare-go/blob/master/docs/conventions.md#methods to get this closer to the library conventions and make our future maintenance a bit easier :)

@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 6, 2023

i've gone through and applied a few updates from https://github.com/cloudflare/cloudflare-go/blob/master/docs/conventions.md#methods to get this closer to the library conventions and make our future maintenance a bit easier :)

Thank you! I just noticed. I don't do a lot of public contribution.. thanks for helping me out :-)

page_shield_connections_test.go Outdated Show resolved Hide resolved
//
// API reference: https://developers.cloudflare.com/api/operations/page-shield-list-page-shield-scripts#Query-Parameters
type ListPageShieldScriptsParams struct {
Direction string `json:"direction"`
Copy link
Member

Choose a reason for hiding this comment

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

similar to the above on query parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here: url.

page_shield_test.go Outdated Show resolved Hide resolved
@jacobbednarz
Copy link
Member

jacobbednarz commented Dec 7, 2023

it looks like the force push has lost my intermediary commits to get the conventions and linter happy.

@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 7, 2023

it looks like the force push has lost my intermediary commits to get the conventions and linter happy.

aaanndd that's why they say don't do a force push... I have your changes in the PR checked out on my machine. Do you want me to close this PR and open a new one?

@jacobbednarz
Copy link
Member

aaanndd that's why they say don't do a force push...

😂 check out https://blog.developer.atlassian.com/force-with-lease/

I have your changes in the PR checked out on my machine. Do you want me to close this PR and open a new one?

do you just want to cherry-pick them onto this one? whatever is easiest for you to do.

@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 7, 2023

To be honest, I have not cherry-picked before. That might cause more issues. I suppose I could try it and then if I screw it up, I have a local tarball of the changes.. I could just try again :-D

page_shield_test.go Outdated Show resolved Hide resolved
@wiremanb
Copy link
Contributor Author

wiremanb commented Dec 7, 2023

@jacobbednarz thanks for the help with this. Seriously, appreciate your patience. I royally screwed this up on the send.

@jacobbednarz jacobbednarz merged commit 1dd0bb9 into cloudflare:master Dec 7, 2023
11 checks passed
@jacobbednarz
Copy link
Member

no worries! thanks for the contribution 🏅

@github-actions github-actions bot added this to the v0.84.0 milestone Dec 7, 2023
github-actions bot pushed a commit that referenced this pull request Dec 7, 2023
Copy link
Contributor

This functionality has been released in v0.84.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants