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.

Sign up for GitHub

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

Jump to bottom

Hard-code CDN for now #5110

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Hard-code CDN for now #5110

merged 1 commit into from
Feb 24, 2020

Conversation

damassi
Copy link
Member

@damassi damassi commented Feb 24, 2020 •
edited
Loading

Noticed an undefined path in the search asset when deploying to prod earlier which has been observed in the past when CDN_URL was not found. We're still not 100% sure about the best way to encode CDN_URL so going to stick with hard-coding for right now.

This has been tested locally with EXPERIMENTAL_APP_SHELL=true and also removed with these commands:

  • yarn start
  • yarn start:prod

See slack convo from #dev where initial issue was reported.

image

(Note the undefined in the asset path)

damassi
damassi commented Feb 24, 2020
View reviewed changes
src/desktop/apps/experimental-app-shell/apps/search/searchMiddleware.tsx
@@ -66,6 +66,7 @@ export const searchMiddleware = async (req, res, next) => {
html: layout,
}
res.status(status).send(layout)
return
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a "cannot send headers" warning in console

@ArtsyOpenSource
Copy link

Warnings
⚠️ The staging version of Metaphysics has breaking changes, this might not affect your build, but could mean your queries don't work the way you expect. See below for breaking changes:
  • ⚠️ Type 'CreateSubmissionMutationInput' was removed
  • ⚠️ Type 'CreateSubmissionMutationPayload' was removed
  • ⚠️ Type 'UpdateSubmissionMutationInput' was removed
  • ⚠️ Type 'UpdateSubmissionMutationPayload' was removed
  • ⚠️ Type 'Asset' was removed
  • ⚠️ Field 'ConsignmentSubmissionConnection.edges' changed type from '[ConsignmentSubmissionEdge]' to '[SubmissionEdge]'
  • ⚠️ Field 'internalID' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'artistID' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'authenticityCertificate' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'dimensionsMetric' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'editionNumber' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'editionSize' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'locationCity' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'locationCountry' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'locationState' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'userID' was removed from object type 'ConsignmentSubmission'
  • ⚠️ Field 'ConsignmentSubmission.category' changed type from 'SubmissionCategoryAggregation' to 'ConsignmentSubmissionCategoryAggregation'
  • ⚠️ Field 'ConsignmentSubmission.edition' changed type from 'Boolean' to 'String'
  • ⚠️ Field 'ConsignmentSubmission.state' changed type from 'SubmissionStateAggregation' to 'ConsignmentSubmissionStateAggregation'
  • ⚠️ Field 'Mutation.createConsignmentSubmission' changed type from 'CreateSubmissionMutationPayload' to 'CreateConsignmentSubmissionPayload'
  • ⚠️ Type for argument 'input' on field 'Mutation.createConsignmentSubmission' changed from 'CreateSubmissionMutationInput!' to 'CreateConsignmentSubmissionInput'
  • ⚠️ Field 'Mutation.updateConsignmentSubmission' changed type from 'UpdateSubmissionMutationPayload' to 'UpdateConsignmentSubmissionPayload'
  • ⚠️ Type for argument 'input' on field 'Mutation.updateConsignmentSubmission' changed from 'UpdateSubmissionMutationInput!' to 'UpdateConsignmentSubmissionInput'
  • ⚠️ Type for argument 'input' on field 'Mutation.addAssetToConsignmentSubmission' changed from 'AddAssetToConsignmentSubmissionInput!' to 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Input field 'submission_id' was added to input object type 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Input field 'gemini_token' was added to input object type 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Input field 'assetType' was removed from input object type 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Input field 'geminiToken' was removed from input object type 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Input field 'submissionID' was removed from input object type 'AddAssetToConsignmentSubmissionInput'
  • ⚠️ Field 'AddAssetToConsignmentSubmissionPayload.asset' changed type from 'Asset' to 'ConsignmentSubmissionCategoryAsset'

Generated by 🚫 dangerJS against ce63c1c

eessex
eessex reviewed Feb 24, 2020
View reviewed changes
Dockerfile
@@ -2,8 +2,6 @@ FROM node:12.14-alpine

WORKDIR /app

ENV CDN_PRODUCTION_URL=https://d1s2w0upia4e9w.cloudfront.net CDN_STAGING_URL=https://d1rmpw1xlv9rxa.cloudfront.net
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax is incorrect, you shouldn't be using an = -- see an example here: https://github.com/artsy/volt/blob/master/Dockerfile#L3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I see this listed both ways in the file - here are the env docs, happy to pair on getting this working, https://docs.docker.com/engine/reference/builder/#environment-replacement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this ever worked on staging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is weird, it worked on staging but not prod?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled that syntax directly from the docs

The second form, ENV = ..., allows for multiple variables to be set at one time. Notice that the second form uses the equals sign (=) in the syntax, while the first form does not. Like command line parsing, quotes and backslashes can be used to include spaces within values.

https://docs.docker.com/engine/reference/builder/#env

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed to:

Screen Shot 2020-02-24 at 10 00 07 AM

Copy link
Member Author

@damassi damassi Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, let me do hard cache clear -- Yup, it works. Going to unset EXPERIMENTAL_APP_SHELL there and see what happens, but don't anticipate a change since the underlying bundle split logic is the same in both cases.

Update: everything looks good.

zephraph
zephraph approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@zephraph zephraph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks for updating it 👍

@artsy-peril artsy-peril bot merged commit 95225ce into artsy:master Feb 24, 2020
@damassi damassi deleted the hard-code-cdn-strings branch February 24, 2020 18:10
@artsy-peril artsy-peril bot mentioned this pull request Feb 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eessex eessex eessex left review comments

@zephraph zephraph zephraph approved these changes

@joeyAghion joeyAghion Awaiting requested review from joeyAghion

@mzikherman mzikherman Awaiting requested review from mzikherman

Assignees

@zephraph zephraph

Labels
Merge On Green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@damassi @ArtsyOpenSource @eessex @zephraph