-
Notifications
You must be signed in to change notification settings - Fork 152
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 graphql validation script to force deploys #3061
Changes from 5 commits
c84a42e
34b25a4
f75e9a1
f2cab46
d8e9c96
5a862ec
a901042
5bdfaa6
e59b7b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
language: node_js | ||
script: yarn danger ci | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @orta I see that in #3061 (comment) you mentioned that danger got removed from CI, but can you elaborate? I'm just wondering if there's a way that we can modify the configuration for Circle than introduce another build system for this project. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, could be done, but it's simpler this way as it doesn't have to run inside docker and Travis is more reliable than Circle for Danger (due to the way they both handle ENV vars) That said, this could also be ported to peril (instead of looking in the node modules, pull out the GraphQL schema from the git repo) but this should be enough for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hah, nice, looks like that is the way to go - the graphql schema for reaction was removed last week, artsy/reaction#1631 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,52 @@ | ||
import { warn, danger } from 'danger' | ||
|
||
// Warn about creating new JS files | ||
const jsFiles = danger.git.created_files.filter(f => f.endsWith('.js')) | ||
if (jsFiles.length) { | ||
const files = danger.github.utils.fileLinks(jsFiles) | ||
warn( | ||
`Please don't include .js files, we want to be using TypeScript found: ${files}.` | ||
import { warn, danger } from "danger" | ||
import { getBreakingChanges } from "./scripts/validate_schemas" | ||
import { BreakingChange } from "graphql" | ||
|
||
// tslint:disable-next-line:no-default-export | ||
export default async () => { | ||
// Warn about creating new JS files | ||
const jsFiles = danger.git.created_files.filter( | ||
f => f.includes("src") && f.endsWith(".js") | ||
) | ||
if (jsFiles.length) { | ||
const files = danger.github.utils.fileLinks(jsFiles) | ||
warn( | ||
`Please don't include .js files, we want to be using TypeScript found: ${files}.` | ||
) | ||
} | ||
|
||
// Breaking change check for Metaphysics production when deploying | ||
if (danger.github.pr.base.ref === "release") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know a conversation topic during our recent platform practice meeting was checking for "dependencies in prod" before code hits staging as well, as such, ensuring that we never have staging code that shouldn't be deployed production. Thoughts on updating this to run on PRs to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm interested in this^ too, but am OK with enforcing this on staging->release merges to start (and we can extend based on how that goes and how often it's an obstacle). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wary that this will "break" builds that have nothing to do with metaphysics, so happy to do this as-is now and can look at making a tighter loop for when people either update reaction or other builds |
||
const breakingChanges = await getBreakingChanges() | ||
const bc = breakingChanges | ||
const typesMissing = bc.filter(b => b.type === "TYPE_REMOVED") | ||
const fieldsMissing = bc.filter(b => b.type === "FIELD_REMOVED") | ||
const unionOptionsMissing = bc.filter( | ||
b => b.type === "TYPE_REMOVED_FROM_UNION" | ||
) | ||
const fieldChanged = bc.filter(b => b.type === "FIELD_CHANGED_KIND") | ||
|
||
const descriptions = { | ||
"Missing types": typesMissing, | ||
"Fields missing": fieldsMissing, | ||
"Fields changed": fieldChanged, | ||
"Union types-mismatch": unionOptionsMissing, | ||
} | ||
|
||
if (bc.length) { | ||
fail( | ||
"Metaphysics production does not have a compatible schema for force." | ||
) | ||
} | ||
|
||
Object.keys(descriptions).forEach(key => { | ||
const breakingChanges: BreakingChange[] = descriptions[key] | ||
if (breakingChanges.length) { | ||
const fields = breakingChanges.map( | ||
b => "`" + b.description.split(" ")[0] + "`" | ||
) | ||
fail(`${key}: ${danger.utils.sentence(fields)}`) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
// @ts-check | ||
|
||
// Grab the schema for reaction out of the node_modules | ||
// to see if it is a subset of the production schema | ||
// | ||
// Used both by Danger during the deploy PR, and also | ||
// before the deployment on circle | ||
// | ||
const { readFileSync } = require("fs") | ||
const { | ||
introspectionQuery, | ||
buildClientSchema, | ||
printSchema, | ||
buildSchema, | ||
findBreakingChanges, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so there's first class support for this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was surprised too! |
||
} = require("graphql") | ||
|
||
const fetch = require("isomorphic-fetch") | ||
|
||
const reactionSchema = readFileSync( | ||
"node_modules/@artsy/reaction/data/schema.graphql", | ||
"utf8" | ||
) | ||
|
||
const metaphysicsProd = "https://metaphysics-production.artsy.net/" | ||
|
||
const downloadSchema = async endpoint => { | ||
const postBody = { | ||
query: introspectionQuery, | ||
operationName: "IntrospectionQuery", | ||
} | ||
|
||
const response = await fetch(endpoint, { | ||
method: "POST", | ||
body: JSON.stringify(postBody), | ||
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
}) | ||
const { data } = await response.json() | ||
// commentDescriptions is hidden | ||
// @ts-ignore | ||
return printSchema(buildClientSchema(data), { commentDescriptions: true }) | ||
} | ||
|
||
const getBreakingChanges = async () => { | ||
const metaphyiscSchema = await downloadSchema(metaphysicsProd) | ||
return findBreakingChanges( | ||
buildSchema(metaphyiscSchema), | ||
buildSchema(reactionSchema) | ||
) | ||
} | ||
|
||
module.exports = { | ||
getBreakingChanges, | ||
} | ||
|
||
// @ts-ignore | ||
if (require.main === module) { | ||
// When this is being called as a script via `node scripts/validate_schemas.js` | ||
getBreakingChanges().then(changes => { | ||
if (changes.length) { | ||
process.exitCode = 1 | ||
console.error( | ||
"Failing due to breaking changes between Force and Metaphysics Production\n\n" | ||
) | ||
console.error(changes) | ||
console.error( | ||
"\n\nYou should deploy metaphysics production, and re-deploy force" | ||
) | ||
} | ||
}) | ||
} |
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.
random q - is there any reason for this diff? Do we prefer single quoted strings in circle ci config files?
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.
prettier does yml files, so these are the same code standard as the rest of front-end apps