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

Mieubrisse/fix dockerfiles #529

Open
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

mieubrisse
Copy link

No description provided.

vgrichina and others added 30 commits August 17, 2020 17:46
Wallet MainNet Release 2020-08-12
Mainnet Wallet Release 2020-08-19
Mainnet Wallet Release 2020-08-26
Mainnet Release 2020-09-02
Mainnet Wallet Release 2020-09-04
Hotfix: fix recovery email formatting
Mainnet Hotfix: 2FA fixes
Mainnet Wallet Release 2020-10-13
Maintenance Release 2020-11-05
Hotfix: Add Moonpay signing endpoint
Hotfix: Change 2FA code expiry time to 30 minutes
Mainnet Release 2021-03-18
Hotfix: Relax requirements for accounts matching public key
MaximusHaximus and others added 21 commits September 9, 2021 19:10
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Just a few questions in-line -- thanks for the contribution, we really appreciate your time :)

@@ -1,5 +1,6 @@
image: gitpod/workspace-postgres
tasks:
# NOTE: As of 2021-1012, the contract helper DB seems to use the indexer DB so this likely won't work anymore
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't true actually -- we use the indexer DB for some things, like finding all accounts for a given public key, or finding recent transactions, but the accounts and recovery methods are entirely a wallet construction, not part of the indexer/on-chain data, and still provided by the local accounts SQL DB instance.

RUN grep -v ACCOUNT_CREATOR_KEY .env.sample | grep -v NODE_URL | grep -v INDEXER_DB_CONNECTION > .env
COPY --from=bridge /root/.near/localnet/node0/validator_key.json .
RUN ACCOUNT_CREATOR_KEY=$(cat validator_key.json | tr -d " \t\n\r") && echo "ACCOUNT_CREATOR_KEY=$ACCOUNT_CREATOR_KEY" >> .env
CMD ["sh", "-c", "sleep 10 && yarn migrate && yarn start"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This yarn migrate command is critical -- without it, the target SQL DB will not be in a sane state, because migrations must be run against the SQL DB before it is usable (they are responsible for creating indexes and creating some tables)

@@ -1,11 +1,8 @@
FROM nearprotocol/bridge as bridge
# TODO pin Node version so the image isn't constantly updating under us?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pinning it to the major version is fine, especially since we intentionally stick to LTS versions (not bleeding edge) where breaking changes are highly unlikely...

@@ -1,5 +1,6 @@
image: gitpod/workspace-postgres
tasks:
# NOTE: As of 2021-1012, the contract helper DB seems to use the indexer DB so this likely won't work anymore
- init: yarn && ./scripts/create_dev_dbs.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this PR deletes this file, but still references it here in the init clause... can you confirm that we don't need create_dev_dbs.sh in the gitpod.yml file? AFAIK it is necessary to configure the Postgres gitpod instance.

@@ -1,9 +0,0 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing this file will break gitpod support; was it not working in the first place? Just want to make sure we're not removing existing functionality :)

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

4 participants