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

Update docker-compose instructions #4077

Merged
merged 2 commits into from
Nov 24, 2021
Merged

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Nov 13, 2021

refs: #4072

Description

The docker-compose.yml file was stuck in time and no longer matched the instructions.

We probably could have a script to update the default version to a specific tag when publishing devnet, but in the meantime, require user to be explicit. The netconfig is also made explicit.

I did not update the AG_SOLO_BASEDIR environment, not sure if that's necessary, but it doesn't match the published https://beta.agoric.net/docker-compose.yml

Security Considerations

N/A

Documentation Considerations

None.

Testing Considerations

This is not part of CI, and I'm not sure how it could be. The change is pretty trivial however.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

This is a good change, and you've pinpointed the needs to update it.

Fwiw, the instructions should point to download the appropriate yml from agoric.net, or at least to set NETCONFIG_URL to the correct location. Maybe it should not default?

@mhofman
Copy link
Member Author

mhofman commented Nov 13, 2021

the instructions should point to download the appropriate yml from agoric.net

The instructions do already point to beta.agoric.net, but not everyone seem to read through the instructions ;)

@mhofman
Copy link
Member Author

mhofman commented Nov 13, 2021

at least to set NETCONFIG_URL to the correct location. Maybe it should not default?

If we can find a way to make it obvious the github hosted file should be edited, removing the default probably is the best approach?

@michaelfig
Copy link
Member

at least to set NETCONFIG_URL to the correct location. Maybe it should not default?

If we can find a way to make it obvious the github hosted file should be edited, removing the default probably is the best approach?

The best I could find was just:

diff --git a/docker/README.md b/docker/README.md
index fd2d9c763..e34c6a68c 100644
--- a/docker/README.md
+++ b/docker/README.md
@@ -2,7 +2,7 @@
 
 ## Quick Start (Overview)
 
- 1. Start the `ag-solo` service: `docker-compose up -d`
+ 1. Start the `ag-solo` service on `devnet`: `NETCONFIG_URL=https://devnet.agoric.net/network-config docker-compose up -d`
  2. Watch the logs for registration details: `docker-compose logs -f --tail=50`
  3. Issue an unguessable URL to the wallet: `docker-compose exec ag-solo agoric open --repl`
 
diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml
index 529a92000..7ff74cc99 100644
--- a/docker/docker-compose.yml
+++ b/docker/docker-compose.yml
@@ -16,6 +16,8 @@ services:
       - setup
       - --webhost=0.0.0.0
       - --webport=${PORT:-8000}
-      - --netconfig=${NETCONFIG_URL:-https://devnet.agoric.net/network-config}
+      # NOTE: Use an environment variable like:
+      #   NETCONFIG_URL=https://devnet.agoric.net/network-config
+      - --netconfig=${NETCONFIG_URL}
 volumes:
   ag-solo-state:

If you don't have time for this or feel it might be misleading, please land without. I'm fine with this PR either way.

@mhofman
Copy link
Member Author

mhofman commented Nov 23, 2021

Should the SDK_TAG not default to anything either then ?

@michaelfig
Copy link
Member

Should the SDK_TAG not default to anything either then ?

Yes, it probably shouldn't default also.

@mhofman mhofman force-pushed the mhofman/4072-docker-compose-tag branch from 228b9b3 to 087e27f Compare November 23, 2021 23:45
@mhofman
Copy link
Member Author

mhofman commented Nov 23, 2021

I ended up doing a little more heavy editing of the Readme. PTAL @michaelfig

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM. Well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants