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

Support local manifest to be pinned and released #81

Merged
merged 4 commits into from
Feb 17, 2020

Conversation

corydickson
Copy link
Contributor

@corydickson corydickson commented Feb 6, 2020

What was wrong?

Issue #60

How was it fixed?

  • Added --manifest_path cli option
  • Automatically pins file if given
  • Updated logging
  • Test coverage

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 corydickson force-pushed the release-local-manifests branch from 9757cdf to cecaf2d Compare February 6, 2020 21:49
@corydickson
Copy link
Contributor Author

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.

Copy link
Contributor

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Thanks for the pr! Just a couple adjustments I'd like to happen. And you're right about testing. I've always just been testing release by locally running commands - which is obviously a horrible way to test 😬 . I'm not sure it's the highest priority, but if you're up for a challenge - adding tests for release would be wonderful! The pm module in web3 would be a good place to get an idea on how to get the chain up & running.

ethpm_cli/parser.py Outdated Show resolved Hide resolved
ethpm_cli/parser.py Show resolved Hide resolved
ethpm_cli/parser.py Outdated Show resolved Hide resolved
@corydickson
Copy link
Contributor Author

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

@corydickson corydickson force-pushed the release-local-manifests branch 2 times, most recently from ef23121 to cecaf2d Compare February 11, 2020 08:00
ethpm_cli/parser.py Outdated Show resolved Hide resolved
@njgheorghita
Copy link
Contributor

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 corydickson force-pushed the release-local-manifests branch 2 times, most recently from 250b9ee to 3ff3320 Compare February 13, 2020 01:21
@corydickson corydickson force-pushed the release-local-manifests branch from 3ff3320 to c176265 Compare February 13, 2020 01:22
@corydickson
Copy link
Contributor Author

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
Copy link
Contributor

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

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

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

@corydickson
Copy link
Contributor Author

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

@corydickson
Copy link
Contributor Author

@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 njgheorghita merged commit fb4c666 into ethpm:master Feb 17, 2020
@njgheorghita
Copy link
Contributor

@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.

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

Successfully merging this pull request may close these issues.

2 participants