Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add --url option to the truffle migrate command #5739

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

sukanyaparashar
Copy link
Contributor

@sukanyaparashar sukanyaparashar commented Nov 29, 2022

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 -

  1. Run a ganache instance in a terminal.
  2. 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.

@sukanyaparashar sukanyaparashar added enhancement doc-change-required Issue indicates a change to documentation is required labels Nov 29, 2022
packages/core/lib/commands/migrate/meta.js Outdated Show resolved Hide resolved
"Creates a provider using the given url and connects to the network."
},
{
option: "--network",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okie. thank you for the explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

packages/core/lib/commands/migrate/meta.js Outdated Show resolved Hide resolved
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@sukanyaparashar
Copy link
Contributor Author

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.

throw new TruffleError(message);
}

let config = loadConfig(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use const here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Yes, we can use const here.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 49 to 58
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}`);
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's add tests for these two

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@dongmingh dongmingh left a comment

Choose a reason for hiding this comment

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

I ran some tests and see the results:

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

  2. 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
Copy link
Contributor Author

I ran some tests and see the results:

  1. start ganache in one terminal, and run truffle migrate --url http://127.0.0.1:8545 on the other terminal, the migration works properly.
  2. 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
Copy link
Contributor Author

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

@eggplantzzz eggplantzzz marked this pull request as draft February 28, 2023 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
doc-change-required Issue indicates a change to documentation is required enhancement wait to merge ⏸
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants