-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handle package webhook #1569
Handle package webhook #1569
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1569 +/- ##
==========================================
+ Coverage 67.90% 67.95% +0.04%
==========================================
Files 95 96 +1
Lines 8750 8762 +12
==========================================
+ Hits 5942 5954 +12
Misses 1898 1898
Partials 910 910
Continue to review full report at Codecov.
|
github/packages.go
Outdated
Prerelease *bool `json:"prerelease,omitempty"` | ||
CreatedAt *Timestamp `json:"created_at,omitempty"` | ||
UpdatedAt *Timestamp `json:"updated_at,omitempty"` | ||
PackageFiles *[]PackageFile `json:"package_files,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 think the type should be []*PackageFile
. Wait for somebody else to confirm it though.
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.
Yes, deifinitely. Since a slice is already a reference type, we don't typically use pointers to slices.
Please change this to []*PackageFile
.
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.
Thank you, @KrammerJake and @jeet-parekh!
This looks great! Just a couple minor tweaks, please, and then we will be ready for a second LGTM and merging.
github/packages.go
Outdated
Prerelease *bool `json:"prerelease,omitempty"` | ||
CreatedAt *Timestamp `json:"created_at,omitempty"` | ||
UpdatedAt *Timestamp `json:"updated_at,omitempty"` | ||
PackageFiles *[]PackageFile `json:"package_files,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.
Yes, deifinitely. Since a slice is already a reference type, we don't typically use pointers to slices.
Please change this to []*PackageFile
.
github/packages.go
Outdated
DownloadURL *string `json:"download_url,omitempty"` | ||
ID *int64 `json:"id,omitempty"` | ||
Name *string `json:"name,omitempty"` | ||
Sha256 *string `json:"sha256,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.
Since "SHA" is "Secure Hash Algorithm", let's please make these "SHA256" and "SHA1".
Repo *Repository `json:"repository,omitempty"` | ||
Org *Organization `json:"organization,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.
Use full names here? Repository and Organization instead of Repo and Org. For the sake of consistency.
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.
This file does have mixed naming conventions where both the full name and shorthand are used; however, the shorthand (Repo
and Org
) versions appear to be the more popular names in this file. I think we can leave these the way they are unless we're trying to standardize to the full name going forward.
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.
Yes, we have not been totally consistent in this regard, so I didn't comment on these. I think that these are the two names we regularly allow to be shortened, as they are globally well known throughout the package and have very small chance of ever causing conflicts with new JSON tags that might show up in later releases.
I'm fine with leaving them as Repo
and Org
.
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.
Thank you, @KrammerJake!
LGTM.
Awaiting second LGTM before merging.
Repo *Repository `json:"repository,omitempty"` | ||
Org *Organization `json:"organization,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.
Yes, we have not been totally consistent in this regard, so I didn't comment on these. I think that these are the two names we regularly allow to be shortened, as they are globally well known throughout the package and have very small chance of ever causing conflicts with new JSON tags that might show up in later releases.
I'm fine with leaving them as Repo
and Org
.
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 👌
Thank you, @wesleimp! |
Addresses #1550