Add --url option to the `truffle migrate` command by sukanyaparashar · Pull Request #5739 · ConsenSys-archive/truffle

sukanyaparashar · 2022-11-29T05:14:18Z

PR description

This PR adds --url option to the truffle migrate command that connects to a specified url. See #5701.

Testing instructions

To test this option -

Run a ganache instance in a terminal.
Run truffle migrate --url http://127.0.0.1:8545 inside a truffle project in a separate terminal. The contracts inside the truffle project will be deployed in Ganache.

Documentation

I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

packages/core/lib/commands/migrate/meta.js

lsqproduction · 2022-11-29T15:27:17Z

packages/core/lib/commands/migrate/meta.js

+          "Creates a provider using the given url and connects to the network."
+      },
+      {
+        option: "--network",


I'm curious about why you prefer --network lives here vs with the other global options.

Actually, can url be a global option as well?

The global options generally appends the options at the end of the usage. The reason behind keeping --network and --url options here rather than in the global options, is to show that they are mutually exclusive.

Okie. thank you for the explanation.

should we add a note in each of these letting the user know that these options are mutually exclusive? do you think that would be helpful?

Usage section shows that they are mutually exclusive but if adding a note in the descriptions is preferable, then we can add it here too.

whatever you think, I don't feel strongly about it :)

packages/core/lib/commands/migrate/meta.js

cds-amal

Thanks, @sukanyaparashar. I have a question about how this feature would work in truffle develop. I suspect we should error better, or not allow the user to enter this command in the REPL. What do you think?

packages/core/lib/commands/migrate/run.js

sukanyaparashar · 2022-11-30T23:48:41Z

Thanks for the approval @lsqproduction. However, I am holding off to merge it now till all the commands have the --url option added to it. I will be opening a PR per command to add the option to it. @cds-amal and I discussed about it and thought that it might be a better idea to add the --url option altogether to all the commands.

eggplantzzz · 2022-12-02T15:53:57Z

packages/core/lib/commands/migrate/run.js

+    throw new TruffleError(message);
+  }
+
+  let config = loadConfig(options);


why not use const here?

Oh! Yes, we can use const here.

cds-amal

I think this command is good, though we should add tests for the REPL command processing.

cds-amal · 2022-12-07T06:45:18Z

packages/core/lib/console.js

+  if (words.includes("--url")) {
+    const message = "url option is not supported within Truffle REPL";
+    return makeIIFE(`ℹ️ : ${message}`);
+  }
+
+  if (words.includes("--network")) {
+    const message = "network option is not supported within Truffle REPL";
+    return makeIIFE(`ℹ️ : ${message}`);
+  }
+


Let's add tests for these two

I will add the tests. Thanks @cds-amal.

The tests should cover both logic branches; with and without the truffle prefix.

dongmingh

I ran some tests and see the results:

start ganache in one terminal, and run truffle migrate --url http://127.0.0.1:8545 on the other terminal, the migration works properly.
start truffle develop and run the following tests:

2.1. migrate --url http://127.0.0.1:8545

            truffle(develop)> migrate --url http://127.0.0.1:8545
            'ℹ️ : url option is not supported within Truffle REPL'

2.2. migrate --network develop

           truffle(develop)> migrate --network develop
           'ℹ️ : network option is not supported within Truffle REPL'

2.3 truffle migrate --network develop
migration works properly

2.4 truffle migrate --url http://127.0.0.1:8545

           truffle(develop)> truffle migrate --url http://127.0.0.1:8545
           Mutually exclusive options, --url and --network detected!
           Please use either --url or --network and try again.
           See: https://trufflesuite.com/docs/truffle/reference/truffle-commands/#migrate

In the last test, the message says both --url and --network are detected though only option --url is provided.

sukanyaparashar · 2022-12-09T17:55:12Z

I ran some tests and see the results:

start ganache in one terminal, and run truffle migrate --url http://127.0.0.1:8545 on the other terminal, the migration works properly.

start truffle develop and run the following tests:

2.1. migrate --url http://127.0.0.1:8545
            truffle(develop)> migrate --url http://127.0.0.1:8545
            'ℹ️ : url option is not supported within Truffle REPL'
2.2. migrate --network develop
           truffle(develop)> migrate --network develop
           'ℹ️ : network option is not supported within Truffle REPL'
2.3 truffle migrate --network develop migration works properly

2.4 truffle migrate --url http://127.0.0.1:8545
           truffle(develop)> truffle migrate --url http://127.0.0.1:8545
           Mutually exclusive options, --url and --network detected!
           Please use either --url or --network and try again.
           See: https://trufflesuite.com/docs/truffle/reference/truffle-commands/#migrate
In the last test, the message says both --url and --network are detected though only option --url is provided.

Thanks for the thorough testing @dongmingh. I will make the required changes to address the issues with truffle migrate command in the REPL.

with truffle prefix without truffle prefix

sukanyaparashar · 2023-01-24T01:14:12Z

Please let me know if I should make a new PR for this work to be completed :)

sukanyaparashar added 2 commits November 28, 2022 21:47

Add --url option to truffle migrate command

7031277

Add integration test for the --url option

89ae0bc

sukanyaparashar added enhancement doc-change-required Issue indicates a change to documentation is required labels Nov 29, 2022

sukanyaparashar requested review from cds-amal, eggplantzzz, haltman-at and lsqproduction November 29, 2022 05:18

lsqproduction reviewed Nov 29, 2022

View reviewed changes

Add spacing in the usage section

a386352

sukanyaparashar requested a review from lsqproduction November 29, 2022 19:39

lsqproduction approved these changes Nov 30, 2022

View reviewed changes

sukanyaparashar added the wait to merge ⏸ label Nov 30, 2022

cds-amal suggested changes Nov 30, 2022

View reviewed changes

packages/core/lib/commands/migrate/run.js Show resolved Hide resolved

eggplantzzz reviewed Dec 2, 2022

View reviewed changes

sukanyaparashar added 2 commits December 5, 2022 20:05

Use const instead of let

afbeb9c

Filter url and network options from the REPL input

8811d74

sukanyaparashar requested review from cds-amal, eggplantzzz and lsqproduction December 6, 2022 03:40

sukanyaparashar added 3 commits December 6, 2022 11:29

Use makeIIFE function instead of TruffleError

d00ba47

Remove extra single quote

d9293fe

Remove extra single quote from develop test case

9aee86c

cds-amal reviewed Dec 7, 2022

View reviewed changes

Add tests to filter url and network options within REPL

b53f4e7

sukanyaparashar requested a review from cds-amal December 8, 2022 03:37

dongmingh reviewed Dec 8, 2022

View reviewed changes

Handle filtering of url and network options

1271865

with truffle prefix without truffle prefix

sukanyaparashar requested a review from dongmingh December 16, 2022 17:48

Refactor function name to convey its actual meaning

4c94755

eggplantzzz assigned cds-amal Jan 31, 2023

eggplantzzz marked this pull request as draft February 28, 2023 18:43

Conversation

sukanyaparashar commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Testing instructions

Documentation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cds-amal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sukanyaparashar commented Nov 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cds-amal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongmingh left a comment

Choose a reason for hiding this comment

Uh oh!

sukanyaparashar commented Dec 9, 2022

Uh oh!

sukanyaparashar commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sukanyaparashar commented Nov 29, 2022 •

edited

Loading