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

Maintain the order from the Gateway resource #1324

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Maintain the order from the Gateway resource #1324

merged 4 commits into from
Dec 14, 2023

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Dec 4, 2023 •
edited

Proposed changes

  • Problem: The status of the Gateway does not correspond to the order of Spec.Listeners.

  • Solution: Explain the approach you took to implement the solution, highlighting any significant design decisions or
    considerations.

  • Testing:

    • Unit test: See [3] (TestBuildGateway) for more details.
    • Manual test: I tested it manually, and I will add more manual test information after receiving early feedback.

Closes #689

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

@netlify Netlify
Copy link

netlify bot commented Dec 4, 2023 •
edited

✅ Deploy Preview for nginx-gateway-fabric ready!

Name Link
🔨 Latest commit 50be6f4
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/657b3cc34b0bb700085f6456
😎 Deploy Preview https://deploy-preview-1324--nginx-gateway-fabric.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kevin85421 kevin85421 changed the title [WIP] Maintain the order from the Gateway resource Maintain the order from the Gateway resource Dec 5, 2023
@kevin85421 kevin85421 marked this pull request as ready for review December 5, 2023 18:04
@kevin85421 kevin85421 requested a review from a team as a code owner December 5, 2023 18:04
@kevin85421
Copy link
Contributor Author

Hi @mpstefan, I have just added a description to this PR and would appreciate some early feedback before dedicating more time to testing and refining it. Although the solution is quite simple, involving only the change of two maps to two slices, this PR affects a large number of files. I am not sure if my solution is too complex or not. Because it is a good first issue, I expect I can fix it in less than 50 LoC. Do I miss anything? Thanks!

kate-osborn
kate-osborn requested changes Dec 5, 2023
View reviewed changes
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.

@kevin85421 thanks for working on this!

The approach looks good to me. I just left a few small suggestions.

internal/mode/static/build_statuses.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/gateway_listener.go Show resolved Hide resolved
internal/mode/static/state/graph/gateway_listener.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/httproute.go Show resolved Hide resolved
@kevin85421
Copy link
Contributor Author

I tested this PR manually 5 times, and all of them worked as expected. I ran kubectl describe gateway $GATEWAY_NAME for each iteration. See gateway_1, gateway_2, gateway_3, gateway_4, and gateway_5 for more details. I will update the commit messages when this PR gets approval.

@sjberman
Copy link
Contributor

sjberman commented Dec 6, 2023

We'll wait to merge this until after our upcoming release.

@kevin85421
Copy link
Contributor Author

Thanks, @kate-osborn and @sjberman! I have already squashed the commits and added a detailed commit message to it. Feel free to let me know if I need to rebase this PR after the release. Thanks!

@kate-osborn kate-osborn added this to the v1.2.0 milestone Dec 6, 2023
@sjberman
Copy link
Contributor

sjberman commented Dec 6, 2023

One small nitpick, can you update your commit message to remove links? Lines change over time and those links won't have much value. It's better just to mention what the issue is and what the solution is.

@kevin85421
Maintain the order from the Gateway resource
0a8ee50 
* Problem: The status of the Gateway does not correspond to the order of `Spec.Listeners`.

* Solution:
  * The order of the `v1.GatewayStatus.Listeners` depends on `GatewayStatus.ListenerStatuses`. However, `GatewayStatus.ListenerStatuses` is a map `map[string]ListenerStatus` which the order of iteration is not guaranteed.
    * [1] This PR changes the type of `GatewayStatus.ListenerStatuses` from map to slice.
  * However, [1] is not enough. The order of `GatewayStatus.ListenerStatuses` is based on the order of `graph.Gateway.Listeners`, but the type of `graph.Gateway.Listeners` is also a map.
    * This PR changes `Gateway.Listeners` from a map to a slice.
@kevin85421
Copy link
Contributor Author

One small nitpick, can you update your commit message to remove links? Lines change over time and those links won't have much value. It's better just to mention what the issue is and what the solution is.

Updated. Thanks!

pleshakov
pleshakov reviewed Dec 13, 2023
View reviewed changes
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.

hi @kevin85421 thanks for the PR! Please see my feedback

internal/mode/static/state/graph/httproute_test.go Outdated Show resolved Hide resolved
internal/framework/status/statuses.go Outdated Show resolved Hide resolved
kevin85421 and others added 2 commits December 13, 2023 22:45
@kevin85421 @pleshakov
Update internal/framework/status/statuses.go
30a78e7 
Improve comments

Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
@kevin85421
Keep values instead of introducing new functions
9e9ecc6 
Keep values instead of introducing new functions
pleshakov
pleshakov approved these changes Dec 14, 2023
View reviewed changes
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 1c4b81a into nginxinc:main Dec 14, 2023
27 checks passed
@lucacome lucacome added the enhancement New feature or request label Mar 13, 2024
@kate-osborn kate-osborn mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pleshakov pleshakov pleshakov approved these changes

@kate-osborn kate-osborn kate-osborn approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
NGINX Gateway Fabric
Status: ✅ Done
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Maintain the order from the Gateway resource
5 participants
@kevin85421 @sjberman @pleshakov @kate-osborn @lucacome