I am fine as long as data is not parsed/filtered within helper method

On Tue, Feb 7, 2017 at 7:48 PM, Omer Akram <email address hidden> wrote:

> Replied inline.
>
> Diff comments:
>
> >
> > === modified file 'tests/utils/snapd.py'
> > --- tests/utils/snapd.py      2017-02-06 14:05:03 +0000
> > +++ tests/utils/snapd.py      2017-02-06 14:05:03 +0000
> > @@ -17,3 +17,138 @@
> >  # You should have received a copy of the GNU General Public License
> >  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >  #
> > +
> > +import getpass
> > +import os
> > +import subprocess
> > +import sys
> > +import shlex
> > +import shutil
> > +import tempfile
> > +
> > +import pexpect
> > +import yaml
> > +
> > +PATH_SNAP = '/usr/bin/snap'
> > +COMMAND_DOWNLOAD = 'download {snap} --channel={channel}'
> > +COMMAND_FIND = 'find {search_term}'
> > +COMMAND_INFO = 'info {snap}'
> > +COMMAND_INSTALL = 'install {snap} --channel={channel}'
> > +COMMAND_LOGIN = 'login {email}'
> > +COMMAND_LOGOUT = 'logout'
> > +COMMAND_REFRESH = 'refresh {snap} --channel={channel}'
> > +CHANNEL_STABLE = 'stable'
> > +PASSWORD_ROOT = 'changed'
> > +
> > +
> > +def is_root_user():
> > +    """Return bool representing if the current user is root."""
> > +    return os.getuid() == 0
> > +
> > +
> > +def run_snapd_command(parameters, return_output=False, cwd=None):
> > +    """ Run the request snapd cli command.
> > +
> > +    :param parameters: the snapd sub-command and their parameters.
> > +        example: install core --beta
> > +    :param return_output: Whether to return the stdout of the command.
> > +    :param cwd: The current working directory for the command, defaults
> to
> > +        pwd.
> > +    :return: Optionally return the stdout of the command, depending if
> > +        the value of `return_output` was True.
> > +    """
> > +    command = '{} {}'.format(PATH_SNAP, parameters)
> > +    if return_output:
> > +        raw_output = subprocess.check_output(
> > +            shlex.split(command), universal_newlines=True, cwd=cwd)
> > +        return raw_output.strip().split('\n')
> > +    subprocess.check_call(shlex.split(command), cwd=cwd)
> > +
> > +
> > +def login(email, password):
> > +    """Login to snapd.
> > +
> > +    :param email: Ubuntu SSO account email address.
> > +    :param password: Ubuntu SSO account password.
> > +    """
> > +    command = '{} {}'.format(COMMAND_LOGIN, email)
> > +    if is_root_user():
> > +        p = pexpect.spawnu(command, logfile=sys.stdout)
> > +    else:
> > +        p = pexpect.spawnu('{} {}'.format('sudo', command),
> logfile=sys.stdout)
> > +        p.expect_exact(
> > +            '[sudo] password for {}: '.format(getpass.getuser()))
> > +        p.sendline(PASSWORD_ROOT)
> > +    p.expect_exact('Password of "{}": '.format(email))
> > +    p.sendline(password)
> > +    p.wait()
> > +
>
> I added an assert to ensure the login was successful.
>
> > +
> > +def logout():
> > +    run_snapd_command(COMMAND_LOGOUT)
> > +
> > +
> > +def download(snap, channel=CHANNEL_STABLE, path=None):
> > +    """Download the requested snap.
> > +
> > +    :param snap: name of the snap to download.
> > +    :param channel: name of the release channel to download from.
> > +    :param path: The path to which the downloaded snap be saved to,
> defaults
> > +        to pwd.
> > +    """
> > +    command = COMMAND_DOWNLOAD.format(snap=snap, channel=channel)
> > +    if path and os.path.isdir(path):
> > +        with tempfile.TemporaryDirectory() as tempdir:
> > +            run_snapd_command(command, cwd=tempdir)
> > +            for file in os.listdir(tempdir):
> > +                shutil.move(os.path.join(tempdir, file), path)
> > +    else:
> > +        run_snapd_command(command)
> > +
> > +
> > +def install(snap, channel=CHANNEL_STABLE):
> > +    """Install the requested snap."""
> > +    run_snapd_command(COMMAND_INSTALL.format(snap=snap,
> channel=channel))
> > +
> > +
> > +def info(snap, verbose=False):
> > +    """Query the Ubuntu store of the information about a snap.
> > +
> > +    :param snap: Name of the snap for which the info is required.
> > +    :param verbose: Whether to information should be detailed.
> > +    :return: Return a dictionary containing information about the snap.
> > +    """
> > +    command = COMMAND_INFO.format(snap=snap)
> > +    if verbose:
> > +        command = ' '.join([command, '--verbose'])
> > +    output = run_snapd_command(command, return_output=True)
> > +    return yaml.load("""{}""".format(output))
> > +
> > +
> > +def refresh(snap, channel=CHANNEL_STABLE):
> > +    """Refresh the requested snap."""
> > +    run_snapd_command(COMMAND_REFRESH.format(snap=snap,
> channel=channel))
> > +
> > +
> > +def find(keyword, **client_side_filters):
> > +    """Find snaps based on the provided filters
> > +
> > +    :param keyword: Keyword to use for the query.
> > +    :param client_side_filters: Whether to filter the results based on
> > +        additional constraints that are not supported by the server.
> > +    :return: Return a list of dictionaries containing information about
> > +        snaps matching the find criteria.
> > +    """
> > +    raw_output = run_snapd_command(
> > +        COMMAND_FIND.format(search_term=keyword), return_output=True)
> > +    headers = [header.lower() for header in raw_output.pop(0).split()]
> > +    organized_data = [dict(zip(headers, line.split())) for line in
> raw_output]
> > +    if len(client_side_filters) > 0:
> > +        filtered_result = organized_data.copy()
> > +        for item in organized_data:
> > +            for key, value in client_side_filters.items():
> > +                if item.get(key) != value:
> > +                    filtered_result.remove(item)
> > +                    continue
> > +        return filtered_result
> > +    return organized_data
>
> We might want to consider making an exception in this case, the raw data
> is returned in the form:
>
> Name                Version  Developer  Notes  Summary
> hello-world-om26er  0.2      om26er     -      A great snap
> crossbar-om26er     0.15.0   om26er     -      Crossbar.io - WAMP
> application router
> omegat-cat          4.1.0-1  jz         -      OmegaT, the free (GPL)
> translation memory tool
> oman                0.1      kalikiana  -      Read manpages from the web
> on your local machine
>
> This is find for human eyes but each test would have to loop through lines
> and get what its looking for, on the contrary what our method does is, it
> returns items in the form:
>
> [{'summary': 'A', 'name': 'hello-world-om26er', 'version': '0.2',
> 'developer': 'om26er', 'notes': '-'}, {'summary': 'Crossbar.io', 'name':
> 'crossbar-om26er', 'version': '0.15.0', 'developer': 'om26er', 'notes':
> '-'}, {'summary': 'OmegaT,', 'name': 'omegat-cat', 'version': '4.1.0-1',
> 'developer': 'jz', 'notes': '-'}, {'summary': 'Read', 'name': 'oman',
> 'version': '0.1', 'developer': 'kalikiana', 'notes': '-'}]
>
> Note: no data loss, rather the same data, but serialized. We can also call
> find('om26er', developer='om26er') and it will locally filter the results
> and look of the form:
>
> [{'summary': 'A', 'name': 'hello-world-om26er', 'version': '0.2',
> 'developer': 'om26er', 'notes': '-'}, {'summary': 'Crossbar.io', 'name':
> 'crossbar-om26er', 'version': '0.15.0', 'developer': 'om26er', 'notes':
> '-'}]
>
> Now if we really have a requirement to return raw data, we could probably
> add a raw=True default parameter to the method so that it returns raw text
> by default and optionally returns serialized data. (Or the other way
> around). Thoughts ?
>
>
>
> --
> https://code.launchpad.net/~canonical-platform-qa/snappy-
> ecosystem-tests/helpers_for_snapd_cli/+merge/316459
> You are reviewing the proposed merge of lp:~canonical-platform-qa/
> snappy-ecosystem-tests/helpers_for_snapd_cli into
> lp:snappy-ecosystem-tests.
>