Code review comment for lp:~canonical-platform-qa/snappy-ecosystem-tests/helpers_for_snapd_cli

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