Skip to content
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

Adds a check for PRs on Metaphysics that production is up-to-date with staging for dependent repos #86

Closed
wants to merge 5 commits into from

Conversation

orta
Copy link
Contributor

@orta orta commented Dec 12, 2018

Re: artsy/README#109

This is now unblocked because the AWS SDK is now available to Peril
danger/peril@a46c62e, and I can use that to grab the images to compare them.

@orta orta mentioned this pull request Dec 19, 2018
@izakp
Copy link
Contributor

izakp commented Jan 16, 2019

Updated upstream/metaphysics.ts default export function to resolve the Git SHA1 tag from based on the "production" tag from ECR and return the MP schema at this ref.

Because the aws-sdk isn't written in Typescript and doesn't define any interfaces for its methods, I had some problems manipulating the data object in the callback of ecr.describeImages and had to update tsconfig.json with:

"noImplicitAny": false,
"strictNullChecks": false

to be able to return 'any' from resolveSHA1ForTag and access data.imageDetails[0] which isn't guaranteed to be non-null (though I do sanity-check the length of data.imageDetails. Not sure what idiomatic Typescript is here.

I'm a bit confused about the scope of what this PR is meant to achieve and what to implement in the the default export function @orta @joeyAghion - should it compare the schemas for Metaphysics at both staging and production (as the title of this PR seems to imply), or should it provide a method to compare another project's schema to Metaphysics' schema at staging and/or production as per artsy/README#109

@orta
Copy link
Contributor Author

orta commented Jan 16, 2019

I think comparing the image tags for production and staging to be the same should probably be enough. I feel like it can skip the schema checking TBH because then it can be expanded to work with any service and not just our GraphQL apis. 👍

@joeyAghion
Copy link
Contributor

I'm not sure I follow the staging/production comparison idea. Requiring Metaphysics' staging and production to be the same is too strict (schema additions in staging should be acceptable). Are you talking about requiring that of upstream APIs (sources)?

The original intent as described in artsy/README#109 was to ensure that the schema required by a PR (according to the committed schema.graphql file in the specified reaction version) was compatible with metaphysics-production's schema (according to an introspection query).

@izakp
Copy link
Contributor

izakp commented Jan 17, 2019

@orta @joeyAghion well, given there is some disagreement here I generalized the upstream/metaphysics module for now to simply export two functions productionSHA1 and stagingSHA1, so they can be used in other checks as required, either comparing staging / production deployments or resolving the schema at this commit and fetching it via danger's github APIs, rather than via an introspection query. It should be as simple as:

import { productionSHA1 } from "upstream/metaphysics"
productionSHA1()
  .then(sha1 => console.log(sha1))
  .catch(err => console.error(err))

@izakp
Copy link
Contributor

izakp commented Jan 17, 2019

Note that this will require IAM credentials present in Danger's runtime environment, with permissions set for the describeImages operation on all our ECR repos. Credentials can be set via the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY env vars.

@izakp
Copy link
Contributor

izakp commented Jan 17, 2019

I feel like this implementation is not addressing artsy/README#109 so I am going to close for now until we can spec a clearer implementation - @orta please don't bother with adding the IAM credentials to Peril's runtime either

@izakp izakp closed this Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants