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

Force Squash nodejs install to use nodesource #5533

Closed
wants to merge 1 commit into from
Closed

Conversation

chosak
Copy link
Member

@chosak chosak commented Aug 22, 2019

Admin assets on Squash PR deployments are currently broken due to a problem running npm. This is the same issue previously reported in #5441. See nodesource/distributions#875 for the rationale behind this workaround; this ensures that nodejs is installed from nodesource instead of from the distro, which may have a newer version.

As part of this change I've restored the Docker image back to python:latest, figuring we might as well do that if we can as opposed to pinning.

Note that the somewhat ugly YAML syntax is needed due to issues including special characters like *&> in YAML.

I've tested this on my fork and it works correctly; hopefully this will work here too, in which case functionality can be tested by visiting the link that Squash posts below and confirming that the admin area properly includes static assets.

Admin assets on Squash PR deployments are currently broken due to a problem running npm. This is the same issue previously reported in PR 5441. See nodesource/distributions issue 875 for the rationale behind this workaround; this ensures that nodejs is installed from nodesource instead of from the distro, which may have a newer version.

As part of this change I've restored the Docker image back to `python:latest`, figuring we might as well do that if we can as opposed to pinning.

Note that the somewhat ugly YAML syntax is needed due to issues including special characters like *&> in YAML.
@squash-labs
Copy link

squash-labs bot commented Aug 22, 2019

Manage this branch in Squash

Test this branch here: https://fix-squash-npm-f8o22.squash.io

@chosak chosak changed the title Force nodejs install to use nodesource Force Squash nodejs install to use nodesource Aug 22, 2019
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Thank you @chosak 🙂

The Squash team let me know they were working on a fix for this, so I’m not sure if we want to make this change. The risk I see in not pinning the dependency is that we’ll have build steps that will now be specific to Debian Buster, without actually explicitly depending on it. Same for the Python version – which is pinned everywhere else in our infrastructure, Circle CI, vagrant-wagtail-develop, and bakerydemo.

What do you think?

@thibaudcolas
Copy link
Member

Yes, apparently this is now fixed on the Squash side. Should we update to python:3.7.4-buster and keep your fix in?

@chosak
Copy link
Member Author

chosak commented Aug 22, 2019

@thibaudcolas thanks, I didn't realize that we were pinning everywhere else. In that case it makes sense to pin here as well. When you say that this is fixed on the Squash side, what does that mean? Will npm work without the fix here? If so, we can close this. If not, let's pin and merge.

@chosak
Copy link
Member Author

chosak commented Aug 26, 2019

However it's been fixed on the Squash side, it looks like admin assets are working again, for example in #5538 opened yesterday. So this PR may no longer be needed. Closing unless/until such time as we need to revisit.

@chosak chosak closed this Aug 26, 2019
@chosak chosak deleted the fix-squash-npm branch August 26, 2019 20:31
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.

None yet

2 participants