Support updates to Marketplace APIs and webhook payload#1042
Conversation
googlebot
added
the
cla: yes
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @srmocher!
This looks like a good start. Feel free to use multiple PRs to address all the issues in #1040.
I've requested a few tweaks.
Also, please make sure to read CONTRIBUTING.md which will show you how to update the github-accessors.go file which should cause the Travis CI build to succeed.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @srmocher - this is a quick review... I need to dig into the history later to see why some things were *string when they look like they should be *Timestamp from a quick glance...
but here are some comments that we can discuss.
| PriceModel *string `json:"price_model,omitempty"` | ||
| UnitName *string `json:"unit_name,omitempty"` | ||
| Bullets *[]string `json:"bullets,omitempty"` | ||
| State *string `json:"state,omitempty"` |
There was a problem hiding this comment.
What are the possible values of State? Can you please add a comment to State?
There was a problem hiding this comment.
I only found sparse description for the possible values here. Seems like it can take either "draft" or "published". https://developer.github.com/marketplace/listing-on-github-marketplace/setting-a-github-marketplace-listing-s-pricing-plan/#creating-pricing-plans
| Plan *MarketplacePlan `json:"plan,omitempty"` | ||
| Account *MarketplacePlanAccount `json:"account,omitempty"` | ||
| OnFreeTrial *bool `json:"on_free_trial,omitempty"` | ||
| FreeTrialEndsOn *string `json:"free_trial_ends_on,omitempty"` |
There was a problem hiding this comment.
This should probably be a *Timestamp.
| type MarketplacePurchase struct { | ||
| // BillingCycle can be one of the values "yearly", "monthly" or nil. | ||
| BillingCycle *string `json:"billing_cycle,omitempty"` | ||
| NextBillingDate *string `json:"next_billing_date,omitempty"` |
There was a problem hiding this comment.
This should probably be a *Timestamp.
|
|
||
| // MarketplacePendingChange represents a pending change to a GitHub Apps Marketplace Plan. | ||
| type MarketplacePendingChange struct { | ||
| EffectiveDate *time.Time `json:"effective_date,omitempty"` |
There was a problem hiding this comment.
This should be a *Timestamp to be consistent with the repo.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @srmocher!
LGTM.
Awaiting second LGTM before merging.
Note to self: This is a breaking API change due to the string => Timestamp and the time.Time => Timestamp changes. So the version number will need to be bumped after merging.
gauntface
left a comment
gauntface
left a comment
There was a problem hiding this comment.
Looks great to me and thank you for the extra timestamp clean up :)
|
Thank you, @gauntface! |
4 participants
Adds support for pending changes to Marketplace plans which were added to the API. (discussed here https://developer.github.com/changes/2018-10-31-improved-experience-for-marketplace-pending-changes/) and referenced here #1040.