Add utils to translate custom types #1310

abhijit-dev82 · 2022-08-11T05:25:12Z

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

k8s-ci-robot · 2022-08-11T05:25:20Z

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.

k8s-ci-robot · 2022-08-11T05:25:25Z

[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.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robscott

Thanks @abhijit-dev82! This looks like a great start. Will discuss this at the community meeting on Monday to ensure that at least some implementations will actually make use of this. This seems like it could be very helpful, but don't want to introduce extra code to maintain until we're sure it will be used.

robscott · 2022-08-11T17:22:09Z

apis/v1alpha2/helper.go

+	return portNum
+}
+
+func ListenerHostnameToPtr(host string) *Hostname {


Maybe remove Listener prefix here since Hostname can be used elsewhere.

robscott · 2022-08-13T00:09:09Z

apis/v1beta1/helper.go

@@ -0,0 +1,177 @@
+/*
+Copyright 2020 The Kubernetes Authors.


Suggested change

Copyright 2020 The Kubernetes Authors.

Copyright 2022 The Kubernetes Authors.

robscott · 2022-08-13T00:09:18Z

apis/v1alpha2/helper.go

@@ -0,0 +1,177 @@
+/*
+Copyright 2020 The Kubernetes Authors.


Suggested change

Copyright 2020 The Kubernetes Authors.

Copyright 2022 The Kubernetes Authors.

robscott · 2022-08-13T00:10:26Z

apis/v1beta1/helper.go

+	return portNum
+}
+
+func ListenerHostnameToPtr(host string) *Hostname {


Maybe remove Listener prefix here since Hostname can be used elsewhere.

robscott · 2022-08-13T00:11:27Z

apis/v1beta1/helper.go

+
+package v1beta1
+
+func SectionNameToPtr(sectionName string) *SectionName {


I think most go linting will usually fail without comments describing each exported function

carlisia · 2022-08-16T18:45:55Z

@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.

robscott · 2022-08-16T19:01:07Z

I really like @carlisia's idea in #1336. @abhijit-dev82 do you think you could incorporate that restructuring into this PR?

/ok-to-test

carlisia · 2022-08-16T23:52:59Z

There is one more input I'd like to offer. If we look at the existing conversion helpers (PathMatchTypePtr and PortNumberPtr here:

gateway-api/apis/v1alpha2/validation/util/utils.go

Line 24 in 854e2bf

func PathMatchTypePtr(s string) *gatewayv1a2.PathMatchType {

...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

carlisia · 2022-08-17T17:45:26Z

This PR adds the directory structure mentioned above: #1337.

abhijit-dev82 · 2022-08-22T13:03:28Z

@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 .

k8s-ci-robot · 2022-08-22T15:28:26Z

@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.

Add utils to translate custom types

k8s-ci-robot · 2022-08-22T16:22:40Z

@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	`b1640c1`	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.

abhijit-dev82 · 2022-08-22T16:23:14Z

HI @carlisia & @robscott , Had to open a new PR as was facing merge issues with the older branch. Below is the link
#1352

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2022

k8s-ci-robot requested review from bowei and hbagdi August 11, 2022 05:25

robscott reviewed Aug 13, 2022

View reviewed changes

carlisia mentioned this pull request Aug 16, 2022

Consider a minor restructure of directory/file for util things #1336

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 16, 2022

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2022

Add utils to translate custom types

b1640c1

Add utils to translate custom types

abhijit-dev82 force-pushed the test-name-fix branch from 445eb1f to b1640c1 Compare August 22, 2022 15:30

abhijit-dev82 force-pushed the test-name-fix branch 2 times, most recently from a012b32 to b1640c1 Compare August 22, 2022 16:17

k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 22, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 22, 2022

abhijit-dev82 closed this Aug 25, 2022

abhijit-dev82 deleted the test-name-fix branch August 25, 2022 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add utils to translate custom types #1310

Add utils to translate custom types #1310

abhijit-dev82 commented Aug 11, 2022

k8s-ci-robot commented Aug 11, 2022

k8s-ci-robot commented Aug 11, 2022

robscott left a comment

robscott Aug 11, 2022

abhijit-dev82 Aug 17, 2022

robscott Aug 13, 2022

abhijit-dev82 Aug 17, 2022

robscott Aug 13, 2022

abhijit-dev82 Aug 17, 2022

robscott Aug 13, 2022

robscott Aug 13, 2022

carlisia commented Aug 16, 2022 •

edited

Loading

robscott commented Aug 16, 2022

carlisia commented Aug 16, 2022

carlisia commented Aug 17, 2022

abhijit-dev82 commented Aug 22, 2022

k8s-ci-robot commented Aug 22, 2022

k8s-ci-robot commented Aug 22, 2022

abhijit-dev82 commented Aug 22, 2022

	Copyright 2020 The Kubernetes Authors.
	Copyright 2022 The Kubernetes Authors.


		package v1beta1

		func SectionNameToPtr(sectionName string) *SectionName {

Add utils to translate custom types #1310

Add utils to translate custom types #1310

Conversation

abhijit-dev82 commented Aug 11, 2022

k8s-ci-robot commented Aug 11, 2022

k8s-ci-robot commented Aug 11, 2022

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia commented Aug 16, 2022 • edited Loading

robscott commented Aug 16, 2022

carlisia commented Aug 16, 2022

carlisia commented Aug 17, 2022

abhijit-dev82 commented Aug 22, 2022

k8s-ci-robot commented Aug 22, 2022

k8s-ci-robot commented Aug 22, 2022

abhijit-dev82 commented Aug 22, 2022

carlisia commented Aug 16, 2022 •

edited

Loading