Skip to content

Conversation

pleshakov
Copy link
Contributor

Proposed changes

Problem:

Configuration-related types in the data plane package include Gateway API types.

As a result:

  • We need to make any validation-related assumptions about them (ex. certain fields cannot be not nil) in the config package.
  • It makes it difficult maintain/extend and potentially allow for supporting more routing resource types (outside of Gateway API) in the future.
  • It makes unit tests more complicated in the config package

Solution:

  • Remove Gateway API types from Configuration-related types.
  • Move the types to types.go file.
  • Move converters to convert.go

Testing: Unit testing

Closes #660

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

…e package

Problem:

Configuration-related types in the data plane package include
Gateway API types.

As a result:
- We need to make any validation-related assumptions about them
(ex. certain fields cannot be not nil) in the config package.
- It makes it difficult maintain/extend and potentially allow for
supporting more routing resource types (outside of Gateway API)
in the future.
- It makes unit tests more complicated in the config package

Solution:

- Remove Gateway API types from Configuration-related types.
- Move the types to types.go file.
- Move converters to convert.go

Testing: Unit testing

Closes nginx#660
@pleshakov pleshakov requested a review from a team as a code owner August 15, 2023 19:30
@github-actions github-actions bot added the tech-debt Short-term pain, long-term benefit label Aug 15, 2023
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

LGTM! The tests seem much simpler now 🚀

Copy link
Contributor

@ciarams87 ciarams87 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 requested a review from kate-osborn August 18, 2023 15:44
@pleshakov pleshakov merged commit 73dc8b0 into nginx:main Aug 18, 2023
@pleshakov pleshakov deleted the tech-debt/refactor-dataplane-package branch August 18, 2023 18:17
miledxz added a commit to miledxz/nginx-gateway-fabric that referenced this pull request Jan 14, 2025
…e package (nginx#976)

Problem:

Configuration-related types in the data plane package include
Gateway API types.

As a result:
- We need to make any validation-related assumptions about them
(ex. certain fields cannot be not nil) in the config package.
- It makes it difficult maintain/extend and potentially allow for
supporting more routing resource types (outside of Gateway API)
in the future.
- It makes unit tests more complicated in the config package

Solution:

- Remove Gateway API types from Configuration-related types.
- Move the types to types.go file.
- Move converters to convert.go

Testing: Unit testing

Closes nginx#660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Short-term pain, long-term benefit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure dataplane.Configuration related types don't include v1beta1 types
4 participants