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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


# Install system dependencies
# Add deploy user
RUN apk --no-cache --quiet add \
Expand Down
1 change: 1 addition & 0 deletions src/desktop/apps/experimental-app-shell/apps/search/searchMiddleware.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

} catch (error) {
console.log(error)
next(error)
Expand Down
23 changes: 19 additions & 4 deletions src/desktop/lib/global_client_setup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,27 @@
* Set webpack public-path asset lookup to CDN in production, but only on
* the client, as we use the assetMiddleware helper to map URLs on the server.
* @see https://github.com/artsy/force/blob/master/src/lib/middleware/assetMiddleware.ts
*
* FIXME: Move this into Circle config and or Docker
*/
if (process.env.NODE_ENV === "production") {
__webpack_public_path__ =
(window.location.hostname === "www.artsy.net"
? process.env.CDN_PRODUCTION_URL
: process.env.CDN_STAGING_URL) + "/assets/"
const { hostname } = window.location
let cdnUrl

// Production
if (hostname === "www.artsy.net") {
cdnUrl = "https://d1s2w0upia4e9w.cloudfront.net"

// Localhost
} else if (hostname === "localhost") {
cdnUrl = ""

// Everything else
} else {
cdnUrl = "https://d1rmpw1xlv9rxa.cloudfront.net"
}

__webpack_public_path__ = cdnUrl + "/assets/"
}

import $ from "jquery"
Expand Down
2 changes: 0 additions & 2 deletions webpack/envs/baseConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ exports.baseConfig = {
new webpack.DefinePlugin({
"process.env": {
NODE_ENV: JSON.stringify(NODE_ENV),
CND_PRODUCTION_URL: JSON.stringify(process.env.CND_PRODUCTION_URL),
CDN_STAGING_URL: JSON.stringify(process.env.CDN_STAGING_URL),
},
}),
// Remove moment.js localization files
Expand Down