-
Notifications
You must be signed in to change notification settings - Fork 92
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
Create suite for different app depending on environment #1007
Conversation
Staging instance deployed by Travis CI! |
This is also the last piece of the checks, so resolves #712 |
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 % nits
api/checks/webhook.go
Outdated
checksStagingAppID = int64(21580) // https://github.com/apps/checks-staging-instance | ||
|
||
wptRepoInstallationID = int64(577173) | ||
wptRepoStagingInstallationID = int64(449270) |
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.
Nit: move these constants to api/checks/api.go
where they are used.
|
||
func (s checksAPIImpl) GetWPTRepoAppInstallationIDs() (appID, installationID int64) { | ||
aeID := appengine.AppID(s.ctx) | ||
domainAndID := strings.Split(aeID, ":") |
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.
Nit: maybe a comment to give an example of aeID
(I tend to do this whenever I'm doing non-trivial string manipulation).
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.
Besides, in the method below, you split on "." instead of ":". Can you confirm?
Line 145 in bff764a
version := strings.Split(appengine.VersionID(a.ctx), ".")[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.
Yah, not sure why the inconsistency, but the ID format is documented on the AppID
method itself.
Description
Uses the production wpt.fyi installation if the events are arriving in production.