-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix: use dapp-offer-up by default #8630
Conversation
@mhofman @michaelfig just merging this won't affect the behavior of |
Correct. It would need to be part of a release: cherry picked to release branch and a release being created. Currently we only create releases with chain upgrades. |
const DEFAULT_DAPP_TEMPLATE = 'dapp-fungible-faucet'; | ||
const DEFAULT_DAPP_TEMPLATE = 'dapp-offer-up'; |
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 expect the integration tests would need to be updated as well.
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.
"the integration tests"?
hmm.. I see https://github.com/Agoric/agoric-sdk/blob/master/.github/workflows/integration.yml#L39
programming in yaml... I'm struggling to follow... seems to lead to...
https://github.com/Agoric/agoric-sdk/blob/master/scripts/registry.sh
But I don't see any use of yarn create @agoric/dapp
. What am I missing?
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.
run: scripts/registry.sh test ${{ matrix.cli }} ${{steps.get-branch.outputs.result}} |
Leads to
agoric-sdk/scripts/registry.sh
Line 110 in d61be8d
persistVar AGORIC_INIT_OPTIONS "[\"--dapp-branch=$2\"]" |
The current getting started steps are captured in that script and another.
While neither reference the new dapp-create, that package is just a wrapper for the agoric cli.
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.
another? what other? Perhaps all will become clear if I study some ci logs.
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.
Yes, registry.sh
triggers a yarn command in agoric-cli
after setting up some environment variables
agoric-sdk/scripts/registry.sh
Line 117 in d61be8d
yarn integration-test |
which ultimately bottoms out in https://github.com/Agoric/agoric-sdk/blob/d61be8d7fbda7dd3846cba38f8829e36bfae92a4/packages/agoric-cli/tools/getting-started.js
Testing this change locally Checked out this branch: Modified the version field in Ran Verified that I'm indeed using my local branch:
Ran
|
@@ -103,6 +103,7 @@ export const gettingStartedWorkflowTest = async (t, options = {}) => { | |||
if (process.env.AGORIC_INIT_OPTIONS) { | |||
const opts = JSON.parse(process.env.AGORIC_INIT_OPTIONS); | |||
initOptions.push(...opts); | |||
initOptions['--dapp-template'] = 'dapp-fungible-faucet'; |
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.
Pretty sure I commented on this earlier, but this isn't correct. The option needs to be set earlier so it can be overridden by env options.
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.
@mhofman I ended up updating the parameter in registry.sh
directly. CI seems to be passing. What do you think about this approach?
…te' parameter" This reverts commit 2097951.
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 think changing registry.sh isn't quite right.
scripts/registry.sh
Outdated
@@ -107,7 +107,7 @@ integrationTest() { | |||
persistVar AGORIC_INSTALL_OPTIONS "[\"$DISTTAG\"]" | |||
} | |||
persistVar AGORIC_START_OPTIONS '["--rebuild"]' | |||
persistVar AGORIC_INIT_OPTIONS "[\"--dapp-branch=$2\"]" | |||
persistVar AGORIC_INIT_OPTIONS "[\"--dapp-template=dapp-fungible-faucet\"]" |
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 don't think this is correct. Why remove the dapp-branch option?
@@ -103,6 +103,7 @@ export const gettingStartedWorkflowTest = async (t, options = {}) => { | |||
if (process.env.AGORIC_INIT_OPTIONS) { | |||
const opts = JSON.parse(process.env.AGORIC_INIT_OPTIONS); | |||
initOptions.push(...opts); | |||
initOptions['--dapp-template'] = 'dapp-fungible-faucet'; | |||
} | |||
t.is( | |||
await myMain(['init', ...initOptions, 'dapp-foo']), |
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 think the easiest is to put the default options here
await myMain(['init', ...initOptions, 'dapp-foo']), | |
await myMain(['init', '--dapp-template', 'dapp-fungible-faucet', '--dapp-base', 'https://github.com/Agoric/', ...initOptions, 'dapp-foo']), |
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.
Ohhh I see. Updated the PR to do that and tests are passing now, thank you!
Omitted --dapp-base
because it defaults to "https://github.com/Agoric/"
Ref:
--dapp-base <base-url> find the template relative to <base-url> (default: "https://github.com/Agoric/")
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.
Oh right dapp-offer-up
moved to the Agoric
org, so DEFAULT_DAPP_URL_BASE
no longer changes in this PR!
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.
LGTM!
Please make sure to squash this one (automerge:squash
should do) given the back and forth in the commit history.
* use dapp-game-pieces (in agoric-labs) by default * feat: use dapp-offer-up in @agoric/dapp * fix(integration-test): update initOptions with '--dapp-template' parameter * fix(integration-test): set --dapp-template at registry.sh level * Revert "fix(integration-test): update initOptions with '--dapp-template' parameter" This reverts commit 2097951. * Revert "fix(integration-test): set --dapp-template at registry.sh level" This reverts commit 8ec4ed3. * fix(integration-test): add --dapp-template parameter to myMain command * fixup! fix(integration-test): add --dapp-template parameter to myMain command --------- Co-authored-by: Luqi Pan <luqi@agoric.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* use dapp-game-pieces (in agoric-labs) by default * feat: use dapp-offer-up in @agoric/dapp * fix(integration-test): update initOptions with '--dapp-template' parameter * fix(integration-test): set --dapp-template at registry.sh level * Revert "fix(integration-test): update initOptions with '--dapp-template' parameter" This reverts commit 2097951. * Revert "fix(integration-test): set --dapp-template at registry.sh level" This reverts commit 8ec4ed3. * fix(integration-test): add --dapp-template parameter to myMain command * fixup! fix(integration-test): add --dapp-template parameter to myMain command --------- Co-authored-by: Luqi Pan <luqi@agoric.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* use dapp-game-pieces (in agoric-labs) by default * feat: use dapp-offer-up in @agoric/dapp * fix(integration-test): update initOptions with '--dapp-template' parameter * fix(integration-test): set --dapp-template at registry.sh level * Revert "fix(integration-test): update initOptions with '--dapp-template' parameter" This reverts commit 2097951. * Revert "fix(integration-test): set --dapp-template at registry.sh level" This reverts commit 8ec4ed3. * fix(integration-test): add --dapp-template parameter to myMain command * fixup! fix(integration-test): add --dapp-template parameter to myMain command --------- Co-authored-by: Luqi Pan <luqi@agoric.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
closes: #8690
Description
Change the default dapp used in
yarn create @agoric/dapp
from fungible faucet todapp-offer-up
.cc @sufyaankhan @toliaqat @0xpatrickdev
Security / Scaling / Upgrade Considerations
n/a
Documentation Considerations
Part of a larger docs transition:
Testing Considerations
In due course, we aim to test the new getting started approach in ci. Initially, @kbennett2000 has agreed to do regular manual testing.