ethpm / ethpm-cli

CLI tool for interacting with the ethPM ecosystem.
https://ethpm-cli.readthedocs.io/
MIT License
42 stars 12 forks source link

Support local manifest to be pinned and released #81

Closed corydickson closed 4 years ago

corydickson commented 4 years ago

What was wrong?

Issue #60

How was it fixed?

Need a bit of guidance on where to add tests for this new feature, had trouble finding the tests that cover the default use case. 99% sure this works as intended :)

Also should I add errors/checks if the manifest_json file is malformed in some way? It looks like guards are already in place that ensure it's an existing file.

Cute Animal Picture

put a cute animal picture link inside the parentheses

corydickson commented 4 years ago

After investigating a bit further, I believe that the issue with adding coverage is that we'll need to set up a local chain to test the release cmd properly.

corydickson commented 4 years ago

Over the next few days I'm going to take on setting up that testing environment, thank you for the guidance!

njgheorghita commented 4 years ago

I made a minor update to web3 - which broke some tests here 😬 - so you'll have to rebase off master again to fix the failing tests in circle.

corydickson commented 4 years ago

Running into issues for end-to-end testing the release cmd. Getting a bit of a cryptic error about it not being able to locate the keyfile. Gonna investigate further

njgheorghita commented 4 years ago

@corydickson Hmm - it's probably that the cli is looking for a keyfile - but one hasn't been set. To sign any txs when you use --keyfile-password it needs a valid encrypted keyfile to be stored via ethpm auth. See here and here. I'm guessing you'll have to set up a keyfile for the default web3 account before you can release any packages. LMK if this sounds about right or not

corydickson commented 4 years ago

I think it might have something to do with my environment: inside the test/cli there is a new file called test_release.py with the following snippet:

import pexpect

from ethpm_cli._utils.ipfs import pin_local_manifest
from ethpm_cli.main import ENTRY_DESCRIPTION
from ethpm_cli.commands.auth import (
    get_authorized_private_key,
    get_keyfile_path,
    import_keyfile,
)
# from ethpm_cli.commands.release import release_package

def test_release_local_manifest(test_assets_dir, keyfile_auth, keyfile):
    # Release a package from a local manifest
    child_keyfile = pexpect.spawn(f"ethpm auth --keyfile-path {keyfile}", timeout=15)
    # Anytime you expect from child_keyfile here it fails with EOF

    local_manifest_path = test_assets_dir / "owned" / "1.0.0.json"
    (package_name, package_version, manifest_uri) = pin_local_manifest(local_manifest_path)
    _, _, password = keyfile_auth

    child = pexpect.spawn(f"ethpm release --manifest-path {local_manifest_path} --keyfile-password {password}", timeout=15)
    child.expect(ENTRY_DESCRIPTION)
    child.expect("\r\n")
    # EOF error here 
    child.expect(f"Retrieving manifest info from local file @ {local_manifest_path} ")
    child.expect(f"{package_name} v{package_version} @ {manifest_uri} ")

This looks like the keyfile path is malformed from the POSIX path given see the line before (last 100 chars):

            exc.__cause__ = None # in Python 3.x we can use "raise exc from None"
>           raise exc
E           pexpect.exceptions.EOF: End Of File (EOF). Empty string style platform.
E           <pexpect.pty_spawn.spawn object at 0x10d93aa90>
E           command: /../../../ethpm-cli/venv/bin/ethpm
E           args: ['/../../.../ethpm-cli/venv/bin/ethpm', 'release', '--manifest-path', '/.../.../.../ethpm-cli/tests/core/assets/owned/1.0.0.json', '--keyfile-password', 'bpassword']
E           buffer (last 100 chars): b''
E           before (last 100 chars): b'ytest-of-cory.dickson/pytest-46/test_release_local_manifest0/xdg_ethpmcli_dir/_ethpm_keyfile.json.\r\n'
E           after: <class 'pexpect.exceptions.EOF'>
corydickson commented 4 years ago

I'll try creating a new keyfile and writing it in the xdg test directory

corydickson commented 4 years ago

Fixed had to import the keyfile in the test working through the rest now

corydickson commented 4 years ago

@njgheorghita May I get a final pass on the PR as it stands? I'm having trouble with managing the web3 backends.

Modified the snippet above with a new deployed registry like in the pm module, then added and activated it through the CLI. But even after sending the authorized_address some ETH (currently using the keyfile and keyfile_auth fixture) and calling ethpm release --manifest-path ...:

b'e["error"])\r\nValueError: {\'code\': -32000, \'message\': \'insufficient funds for gas * price + value\'}\r\n'

Assuming this is because the config.w3 is not the same as the new w3 fixture placed in test/cli/conftest.py and the chains are different. Tried sending a transaction through the config backend but it doesn't have access to eth_coinbase and the defaultAccount is an empty object.

You mentioned this wasn't high priority but if you have any suggestions on how to move forward I can continue working on it 🙏

njgheorghita commented 4 years ago

@corydickson Hmm, hard to tell exactly what might be the problem, though your explanation seems plausible - if you want to push your code into a draft pr I'd be happy to take a closer look at it.

In the meantime #88 should be a more exciting and definitely more useful challenge if you're up for it.