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

Add support for canned responses to VS/VSR #780

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Conversation

Rulox
Copy link
Contributor

@Rulox Rulox commented Dec 5, 2019

Proposed changes

This PR adds the "canned responses" features.

Now, it is possible to return preconfigured responses for any VS or VSR with the action return.

Also refactored the redirect action to reuse the same code as the new return action, as both make usage of the "return" directive from NGINX.

@Rulox Rulox added the enhancement Pull requests for new features/feature enhancements label Dec 5, 2019
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@Rulox thanks for the PR. good job!
I left a few suggestions regarding consistency and simplification. Let me know your thoughts.

docs/virtualserver-and-virtualserverroute.md Outdated Show resolved Hide resolved
pkg/apis/configuration/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/configuration/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/configuration/validation/validation.go Outdated Show resolved Hide resolved
internal/configs/virtualserver.go Show resolved Hide resolved
@Rulox Rulox requested a review from pleshakov December 5, 2019 20:49
@Rulox
Copy link
Contributor Author

Rulox commented Dec 5, 2019

@pleshakov

Changed the validateSpecialVar to a switch case, with the next considerations:

isValidSpecialVariableHeader is needed but there's a difference between using - and _. Creating a new regexp for this is against DRY and KISS, reusing the header with replacing the underscores seems a much better and elegant solution to me.

I didn't create isValidSpecialVariableCookie or isValidSpecialArgument because those would have been just wrappers to isCookieName and isArgumentName so I think that would be just a waste of time and add code for nothing.

Let me know what you think thanks

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@Rulox thank for the update.

Please see a few comments below. Additionally, there is a bug. sorry, only noticed that during this review.

Changed the validateSpecialVar to a switch case, with the next considerations:
I didn't create isValidSpecialVariableCookie or isValidSpecialArgument because those would have been just wrappers to isCookieName and isArgumentName so I think that would be just a waste of time and add code for nothing.

the switch looks good along with using the existing functions.

isValidSpecialVariableHeader is needed but there's a difference between using - and _. Creating a new regexp for this is against DRY and KISS, reusing the header with replacing the underscores seems a much better and elegant solution to me.

creating a new function is against DRY but not KISS. I left a comment for the corresponding line explaining why.

pkg/apis/configuration/validation/validation.go Outdated Show resolved Hide resolved
internal/configs/virtualserver.go Show resolved Hide resolved
@Rulox Rulox requested a review from pleshakov December 6, 2019 20:36
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@Rulox please my comments and suggestions

internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved
pkg/apis/configuration/validation/validation_test.go Outdated Show resolved Hide resolved
internal/configs/virtualserver.go Show resolved Hide resolved
@Rulox Rulox requested a review from pleshakov December 12, 2019 12:08
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

👍

@pleshakov pleshakov merged commit 8097840 into master Dec 17, 2019
@pleshakov pleshakov deleted the canned-responses branch December 17, 2019 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants