-
Notifications
You must be signed in to change notification settings - Fork 4
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
877 fix local build: npm certificate, docker-compose platform #878
Conversation
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.
I've successfully built and logged in locally. Suggest dependencies not be pinned like this indefinitely, but not a high priority. Approved!
"vuex": "^3.6.2", | ||
"vuex-multi-tab-state": "^1.0.17", | ||
"vuex-persistedstate": "^4.1.0" | ||
"axios": "0.21.2", |
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.
File a follow-up issue to restore the ^
? I don't want to block this now, but if we anticipate any future work here, it will be useful to be able to distinguish the versions that really do need to be pinned to an exact version from those that can hopefully be upgraded, with the package-lock providing reproducible builds.
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.
That's true, the only problem with that is that if we have to reproduce package-lock.json from package.json (which I had to do to fix this registry issue) then we would have no ability to guarantee the new package-lock.json has the same versions. So not sure if it's worth the risk?
README-DEVELOPMENT.md
Outdated
@@ -15,21 +15,20 @@ This page lists contains rudimentary instructions for building the development e | |||
|
|||
`docker-compose up --build` | |||
|
|||
1. All subsequent commands should be run from the `server` directory | |||
3. All subsequent commands should be run from the `server` directory |
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.
Maybe move the part about starting a new terminal here? This is silly, but I got breakfast and glanced at my screen a couple times, expecting the docker-compose up
to complete and return to the shell, but that's not what it does, obviously.
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.
Or maybe a log message from dpcreator-db-1 when it's warm, with a reminder to run migrations? Probably more trouble than it's worth.
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.
I agree the steps here are confusing - you actually don't have to cd to server
directory because it is running the command inside the container, so I removed that.
dpcreator-server does give you a hint that migrations need to be done.
dpcreator-server-1 | You have 47 unapplied migration(s). Your project may not work properly until you apply the migrations for app(s): account, admin, analysis, auth, authtoken, banner_messages, contenttypes, dataset, dataverses, release_schemas, sessions, sites, socialaccount, terms_of_access, user.
dpcreator-server-1 |
dpcreator-server-1 | Run 'python manage.py migrate' to apply them.
I agree it would be nice to have a reminder at the end of the build.
@@ -43,6 +43,7 @@ services: | |||
# - Built from "server/Dockerfile" | |||
# --------------------------------- | |||
server: | |||
platform: linux/amd64 |
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.
If you understand right now why this is needed, consider adding a note: Someone might want to tweak it in the future and knowing constraints could be useful.
(And if it just works this way, but it's not clear why, no worries!)
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.
This is needed because the new Macs use Apple Silicon chips, which are based on the ARM architecture, and the OpenDP library is not built to run on that. I had that happen last summer, and I updated docker-compose-cypressdev.yml, but I forgot to update docker-compose.yml. I added a comment in the docker compose files about that.
I'm assuming you have a new Mac too and that's why you had the problem :)
@mccalluc thanks for your comments, I made some additional changes |
"docker-compose" -> "docker compose"
…pcreator into 877-certificate-expired
|
Far from best practice but merging since local docker compose is working... |
I made some changes to fix the local build.
For the the npm install errors, I rebuilt package-lock.json to use the npmjs registry (https://registry.npmjs.org/).
I removed the '^' from package.json, so it will use exact versions rather than getting newer versions. It's still hard to exactly reproduce the old package-lock.json, so I think there will be some bugs due to different versions that have to be worked out. For example there is a validation error that happens when building the client. But it still starts up, and the basic workflow works.
The problems we were seeing in Celery and the Django Server were due to a missing platform specification in docker-compose.yml. The OpenDP library needs to run on linux/amd64 platform.