gateway-api
gateway-api copied to clipboard
kubernetes-sigs
Add utils to translate custom types
What type of PR is this? /kind cleanup
What this PR does / why we need it: Proposed change set are helper functions to translate to and from custom types.
Which issue(s) this PR fixes: Fixes #826
Does this PR introduce a user-facing change?: No
Hi @abhijit-dev82. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: abhijit-dev82
Once this PR has been reviewed and has the lgtm label, please assign youngnick for approval by writing /assign @youngnick in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@abhijit-dev82 thank you for doing this, I will certainly use it so I can delete some code in my code base!
Everything lgtm except I'm not loving the name and the location of the file, especially when I consider existing helper functionality.
I opened an issue to explain: Consider a minor restructure of directory/file for util things · Issue #1336 · kubernetes-sigs/gateway-api. I'd hate to block the merging of this PR for something that is sorta trivial but brakes the interface so it will require some thinking. At the same time, I would love adding such helpful code in a place that makes great sense, given the context around it.
I really like @carlisia's idea in #1336. @abhijit-dev82 do you think you could incorporate that restructuring into this PR?
/ok-to-test
There is one more input I'd like to offer. If we look at the existing conversion helpers (PathMatchTypePtr and PortNumberPtr here:
https://github.com/kubernetes-sigs/gateway-api/blob/854e2bfc52764bda4d81dd24851ab124bf564e8f/apis/v1alpha2/validation/util/utils.go#L24
...we see that there is no To or From. That style is idiomatic of Go because it doesn't duplicate information that we can see in a different way. In this case, that PathMatchTypePtr takes a string is indicated in the signature, and it is idiomatic to name the function based on what it returns, in this case the pointer. Sooooo.... long paragraph to suggest changing the names to fit this format:
s/SectionNameToPtr/SectionNamePtr
s/SectionNameFromPtr/SectionNameStr
This PR adds the directory structure mentioned above: https://github.com/kubernetes-sigs/gateway-api/pull/1337?w=1.
@carlisia & @robscott Thank you for all the comments. I was away due to health reasons, hence was not able to make the necessary changes. I see that the directory changes are already done, will add the new file in the util dir. Do we need to see we can auto generate these translation routines based on the custom types defined .
@abhijit-dev82: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@abhijit-dev82: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-gateway-api-verify | b1640c1ae1e2bfe9113fa49ba4ad9b7ced577fcf | link | true | /test pull-gateway-api-verify |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
HI @carlisia & @robscott , Had to open a new PR as was facing merge issues with the older branch. Below is the link https://github.com/kubernetes-sigs/gateway-api/pull/1352