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.

Sign up for GitHub

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

Jump to bottom

Add verification of filter size #846

Closed
dersoi opened this issue Oct 27, 2020 · 7 comments · Fixed by #848
Closed

Add verification of filter size #846

dersoi opened this issue Oct 27, 2020 · 7 comments · Fixed by #848
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. workflow/pending-cloudflare-response Indicates an issue or PR requires a response from the Cloudflare team.

Comments

@dersoi
Copy link

dersoi commented Oct 27, 2020

Current Terraform version

Terraform v0.13.4

Description

The size of a filter is limited to 4096 chars on Cloudflare. This, however, is not checked by the plugin. This means that if you do a 4097 chars filter, it will tell you that it is OK, and proceed to crash at the apply stage.

Use-cases

If you have an automated CI/Pipeline on Git, it is likely that as long as you are not on the master branch, you only do a fmt, validate, and plan. These results are then used to determine if the new modifications can be merged to the master branch. If the filter is more than 4096 (which is frankly difficult to see with the naked eye), it passes all the tests.

  • Please vote on this issue by adding a 👍 reaction
    to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull
    request, please leave a comment
@dersoi dersoi added kind/enhancement Categorizes issue or PR as related to improving an existing feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 27, 2020
@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 27, 2020 •
edited
Loading

I'm on board with this to get earlier feedback in the change lifecycle 👍

We can use a ValidateFunc in the schema to call out to ValidateFilterExpression which will cover all the validations (not just size) and notify of any violations. This also ensures that we aren't duplicating the checks/logic of the Filters API in the Terraform provider.

@dersoi Are you interested in taking a pass at putting up a PR for this one?

@jacobbednarz jacobbednarz added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 27, 2020
jacobbednarz added a commit to jacobbednarz/terraform-provider-cloudflare that referenced this issue Oct 28, 2020
@jacobbednarz
filter: Improve validation of expression
a1e3bf2 
While making changes to filters you need to run `apply` to actually find
out whether the expression is valid. This is a slow feedback cycle for
developers and there is an API endpoint that allows you to validate
expressions before using them so it's a no brainer to improve the
validation in the schema.

This updates the `ValidateFunc` for `expression` to call out to the
validation API endpoint and raise those exceptions earlier in the
development cycle.

Fixes cloudflare#846
@jacobbednarz jacobbednarz mentioned this issue Oct 28, 2020
Merged
jacobbednarz added a commit to jacobbednarz/terraform-provider-cloudflare that referenced this issue Oct 28, 2020
@jacobbednarz
filter: Improve validation of expression
20dc090 
While making changes to filters you need to run `apply` to actually find
out whether the expression is valid. This is a slow feedback cycle for
developers and there is an API endpoint that allows you to validate
expressions before using them so it's a no brainer to improve the
validation in the schema.

This updates the `ValidateFunc` for `expression` to call out to the
validation API endpoint and raise those exceptions earlier in the
development cycle.

Fixes cloudflare#846
@dersoi
Copy link
Author

dersoi commented Oct 28, 2020

@jacobbednarz I see you were faster than I. Thank you! Do you know when the next release wil be ?

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 28, 2020 via email

@dersoi
Copy link
Author

dersoi commented Oct 28, 2020

Awesome thanks!

@jacobbednarz
Copy link
Member

I've reverted #848 and this needs to wait until we API token support on that endpoint.

@jacobbednarz jacobbednarz reopened this Nov 5, 2020
@jacobbednarz jacobbednarz mentioned this issue Nov 5, 2020
Closed
@jacobbednarz jacobbednarz added the workflow/pending-cloudflare-response Indicates an issue or PR requires a response from the Cloudflare team. label Nov 8, 2020
@jacobbednarz
Copy link
Member

This is good to go again now the endpoint supports API tokens

jacobbednarz added a commit that referenced this issue Nov 1, 2021
@jacobbednarz
filter: move validation to schema
3014a32 
While making changes to filters you need to run apply to actually find
out whether the expression is valid. This is a slow feedback cycle for
developers and there is an API endpoint that allows you to validate
expressions before using them so it's a no brainer to improve the
validation in the schema.

This updates the ValidateFunc for expression to call out to the
validation API endpoint and raise those exceptions earlier in the
development cycle.

Take 2 of #848 now that API tokens are supported in the routes.

Closes #846
@jacobbednarz jacobbednarz mentioned this issue Nov 1, 2021
Closed
jacobbednarz added a commit that referenced this issue Nov 1, 2021
@jacobbednarz
filter: move validation to schema
53dfba5 
While making changes to filters you need to run apply to actually find
out whether the expression is valid. This is a slow feedback cycle for
developers and there is an API endpoint that allows you to validate
expressions before using them so it's a no brainer to improve the
validation in the schema.

This updates the ValidateFunc for expression to call out to the
validation API endpoint and raise those exceptions earlier in the
development cycle.

Take 2 of #848 now that API tokens are supported in the routes.

Closes #846
@jacobbednarz
Copy link
Member

closing this off as there isn't a great way to:

  • reuse the existing HTTP client; and
  • pull the credentials using d.Get from the schema validation in question.

may revisit in the future if the schema method recievers change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. workflow/pending-cloudflare-response Indicates an issue or PR requires a response from the Cloudflare team.
Projects
None yet
Milestone
No milestone
2 participants
@jacobbednarz @dersoi