-
Notifications
You must be signed in to change notification settings - Fork 29
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 library functions - 01 #251
obs: add library functions - 01 #251
Conversation
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. |
obs/project.go
Outdated
Architectures []string `json:"arch" xml:"arch"` | ||
ReleaseTargets []ReleaseTarget `json:"releasetarget,omitempty" xml:"releasetarget,omitempty"` | ||
Architectures []string `json:"architectures" xml:"arch"` | ||
ReleaseTargets []ReleaseTarget `json:"releaseTarget,omitempty" xml:"releasetarget,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.
why this change?
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.
@cpanato I proposed this change in the previous PR. JSON works in a bit different way than XML. For example, you don't have arrays in XML, but you have arrays in JSON. Because of that, I would prefer that JSON tags are called same as fields, i.e. plural and camel-case. In my opinion, in the JSON world, architectures
is more natural than arch
/ok-to-test |
/test all |
Update: Ready for the next set of review |
/test all |
1 similar comment
/test all |
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.
Generally looks fine, some minor nits
obs/error.go
Outdated
} | ||
|
||
func (e *APIError) Error() string { | ||
return fmt.Sprintf("HTTP status %d: %s (%s)", e.HTTPStatusCode, e.XMLStatusCode, e.Message) |
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.
Does it make sense to have a different error message if HTTP status code is 0?
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.
we generally have the message as unable to generate the request
.
497bdfb
to
462a5f7
Compare
/test all |
when we are trying to build the Read function for library, what do we actually expect from it? Is it to display the Project meta? |
/test all |
e868220
to
07ec6b9
Compare
/test all |
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 lint error, please address these errors
Signed-off-by: NitishKumar06 <[email protected]>
/test all |
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.
/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: kubernetes/sig-release#2295