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 PartiallyInvalid HTTPRoute Condition #1160

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Oct 18, 2023

Proposed changes

Replace previously used TODO Conditions with PartiallyInvalid HTTPRoute Condition.

Problem: There was no HTTPRoute Condition that conveyed when a Route was PartiallyInvalid.

Solution: Added the PartiallyInvalid HTTPRoute Condition.

Testing: Manually deployed HTTPRoute with invalid and valid rules and checked to see if the HTTPRoute correctly displayed the new condition. Also checked to see if the matches are valid, with an invalid filter, that the HTTPRoute correctly displayed the new condition. Added additional Unit Tests.

Please focus on (optional): Did I update all the documentation? Are the tests that I added all necessary and done correctly?

Closes #485

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@bjee19 bjee19 requested a review from a team as a code owner October 18, 2023 23:46
@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 18, 2023
@bjee19
Copy link
Contributor Author

bjee19 commented Oct 18, 2023

Since we are using our own version of PartiallyInvalid (copied from Gateway sig), should I open up a ticket which is for switching to their definition when they release it in 1.0?

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.

good stuff!
please see a few change requests.
naming suggestions aim to bring consistency with the existing code. for cases, when there is a valid rule(s) but some are invalid, we can use "dropped" in the name of variables and cases in tests.

internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
@bjee19 bjee19 requested a review from pleshakov October 19, 2023 20:33
@bjee19 bjee19 force-pushed the enh/add-partial-validity-condition branch from 9bec288 to 1cf60ce Compare October 19, 2023 21:01
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.

👍

@bjee19 bjee19 merged commit 89f6452 into nginxinc:main Oct 19, 2023
23 checks passed
@bjee19 bjee19 deleted the enh/add-partial-validity-condition branch November 20, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

HTTPRoute partial validity
3 participants