-
Notifications
You must be signed in to change notification settings - Fork 30
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
obs: add initial Project types #246
obs: add initial Project types #246
Conversation
Welcome @nitishfy! |
Hi @nitishfy. 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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question answered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the initial review, I can do another review once these comments are addressed. Looks pretty solid so far!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better, some more nits from my side
I think I can move forward with adding more features now. Let me know if you have any more feedback to address, I'll resolve them along the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final nits for now. Perhaps we can merge this PR once these nits are addressed.
obs/obs.go
Outdated
Title *string `json:"title" xml:"title,omitempty"` | ||
Description *string `json:"description" xml:"description,omitempty"` | ||
URL *string `json:"url" xml:"url,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind about these fields. Let's make this field non-pointer string and remove omitempty
. We're actually going to get the same result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but how is this working internally then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand your question. Can you elaborate please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mhh, why is there need to remove the pointers and omitempty from here? (these fields are after all not mandatory, isn't it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because working with string pointers (or with any other pointer to a primitive type) is a little bit complicated and doesn't provide good user experience. When defining the struct, you have to do something like this:
str := "My Project Name"
p := &Project{
URL: &str,
}
Instead of:
p := &Project{
URL: "My Project Name",
}
When you have multiple strings, like in our case, it gets even worse.
Right now, if you completely omit the URL when defining a project, you'll get XML like this one:
<project>
...
</project>
If you remove pointers and omitempty
, you'll get XML like this one:
<project>
<url></url>
</project>
These two XMLs are practically the same. OBS is even going to remove the empty tags automatically. However, the user experience of using the library is much better in the second case (when not using pointers to primitive types).
/test all |
/ok-to-test |
/assign @saschagrunert @cpanato |
/retitle obs: add initial Project types |
15318a1
to
ad210a9
Compare
Update: PR is ready for the next set of review and probably for merge as well. |
d9afd36
to
3b5dece
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but let's address these nits in a follow-up.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nitishfy, xmudrii The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reference: #2295
This PR is created with the purpose of adding the initial project structure for the OBS project meta file for golang library