Merge lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh into lp:snappy-ecosystem-tests

[Omer Akram]

Export staging variables over ssh.

Ensure staging environment variables are available over ssh.

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:24
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/226/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2027/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/226/rebuild

review: Approve (continuous-integration)

[Omer Akram (om26er)]

By default ssh is secure and does not allow any random environment variable to be exported. So the ssh config within the container also needs to be updated to allow these variables to be exported.

That can be enabled by container setup script. We basically need to append staging environment variable names to AcceptEnv key inside /etc/ssh/sshd_config. OR if we just want to put this variables inside /etc/environment while we setup the container, that would work, though I am not a fan of that.

For testing just update the AcceptEnv line in the container to

AcceptEnv *

[Omer Akram (om26er)]

(and restart the container)

[platform-qa-bot (platform-qa-bot)]

FAILED: Continuous integration, rev:25
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/228/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2029/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/228/rebuild

review: Needs Fixing (continuous-integration)

[lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh]

26. By Omer Akram on 2017-03-02

Finish rebase

27. By Omer Akram on 2017-03-02

fix

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:27
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/229/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2030/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/229/rebuild

review: Approve (continuous-integration)

[Heber Parrucci (heber013)]

Comment inline

review: Needs Fixing

[lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh]

28. By Omer Akram on 2017-03-02

fix per suggestion

[Omer Akram (om26er)]

replied inline.

[platform-qa-bot (platform-qa-bot)]

FAILED: Continuous integration, rev:28
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/234/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2038/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/234/rebuild

review: Needs Fixing (continuous-integration)

[lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh]

29. By Omer Akram on 2017-03-02

merge with pre-req

[platform-qa-bot (platform-qa-bot)]

FAILED: Continuous integration, rev:29
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/235/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2040/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/235/rebuild

review: Needs Fixing (continuous-integration)

[lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh]

30. By Omer Akram on 2017-03-02

ignore arguments-differ pylint warning

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:30
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/236/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2041/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/236/rebuild

review: Approve (continuous-integration)

[Heber Parrucci (heber013)]

Comments inline. Thanks

review: Needs Fixing

[I Ahmad (iahmad)]

+1 for heber's recommendation, env should not be hard coded rather
configurable based on value in config file

On Fri, Mar 3, 2017 at 2:29 AM, Heber Parrucci <<email address hidden>
> wrote:

> Review: Needs Fixing
>
> Comments inline. Thanks
>
> Diff comments:
>
> >
> > === modified file 'snappy_ecosystem_tests/utils/ssh.py'
> > --- snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > +++ snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > @@ -31,13 +33,13 @@
> > class SSHClient(paramiko.SSHClient):
> > """Extended SSHClient."""
> >
> > - def exec_command(self, command, bufsize=-1, timeout=None,
> get_pty=False,
> > - cwd=''):
> > + def exec_command(self, command, bufsize=-1, timeout=None,
> get_pty=False, # pylint: disable=arguments-differ
>
> If we disable the rules every time pylint warns us about something, we are
> losing the tool benefits and would be the same as not using it at all.
> In this particular case you are changing the parent method signature which
> could be ok in some cases. But if we look deeper in the problem, you have
> created an SSHClient and overrided exec_command just to change the dir when
> executing the command. I think this is not necessary at all... You could
> use the paramiko function and achieve the same. And the trick will be in
> the helper side... For example in case of snapd:
> http://paste.ubuntu.com/24098212/
>
> > + environment=None, cwd=''):
> > """Execute the given command over ssh."""
> > if cwd:
> > command = 'cd {}; {}'.format(cwd, command)
> > _, stdout, stderr = super().exec_command(command, bufsize,
> timeout,
> > - get_pty)
> > + get_pty, environment)
> > if stdout.channel.recv_exit_status() != 0:
> > raise ValueError(stderr.read().decode().strip())
> > response = stdout.read().decode().strip()
> > @@ -118,4 +120,9 @@
> > _hostname, _username, _port = get_snapd_remote_host_credentials()
> > ssh = SSHManager.get_instance(
> > hostname or _hostname, username or _username, port or _port)
> > - return ssh.exec_command(command, cwd=cwd)
> > + return ssh.exec_command(
> > + command,
> > + cwd=cwd,
> > + environment=STAGING_VARIABLES if get_current_store() ==
> 'staging'
>
> It is almost the same than before the fix. What I meant when wrote the
> suggestion is to dynamically pass the corresponding environment variables
> depending on the current store. You could have a function that returns the
> corresponding dictionary containing the variables and receives the store
> name as parameter. So in the future if we need to add variables for example
> for production, we just create the dictionary and that's all, we don't need
> to change this method for that.
>
> > + else None
> > + )
>
>
> --
> https://code.launchpad.net/~canonical-platform-qa/snappy-
> ecosystem-tests/export-staging-variables-ssh/+merge/318743
> You are subscribed to branch lp:snappy-ecosystem-tests.
>

[Omer Akram (om26er)]

replied inline.

[I Ahmad (iahmad)]

There could be situation where container is previously set to staging but
now needs to be reset to the production (in cases where environment needs
to be reused).

On Fri, Mar 3, 2017 at 4:48 PM, Omer Akram <email address hidden> wrote:

> replied inline.
>
> Diff comments:
>
> >
> > === modified file 'snappy_ecosystem_tests/utils/ssh.py'
> > --- snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > +++ snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > @@ -31,13 +33,13 @@
> > class SSHClient(paramiko.SSHClient):
> > """Extended SSHClient."""
> >
> > - def exec_command(self, command, bufsize=-1, timeout=None,
> get_pty=False,
> > - cwd=''):
> > + def exec_command(self, command, bufsize=-1, timeout=None,
> get_pty=False, # pylint: disable=arguments-differ
>
> pylint is picky, yes. But since I am trying to extend the functionality of
> a method, I added a new parameter, quite normal.
>
> pylint threw that warning because it could easily be a mistake on my end,
> but given its intentional, I had to silence pylint for that. The reason I
> added that override was inspired by the conversation I found on pylint'
> mailing list[1].
>
> [1] http://code.activestate.com/lists/python-list/188090/
>
> > + environment=None, cwd=''):
> > """Execute the given command over ssh."""
> > if cwd:
> > command = 'cd {}; {}'.format(cwd, command)
> > _, stdout, stderr = super().exec_command(command, bufsize,
> timeout,
> > - get_pty)
> > + get_pty, environment)
> > if stdout.channel.recv_exit_status() != 0:
> > raise ValueError(stderr.read().decode().strip())
> > response = stdout.read().decode().strip()
> > @@ -118,4 +120,9 @@
> > _hostname, _username, _port = get_snapd_remote_host_credentials()
> > ssh = SSHManager.get_instance(
> > hostname or _hostname, username or _username, port or _port)
> > - return ssh.exec_command(command, cwd=cwd)
> > + return ssh.exec_command(
> > + command,
> > + cwd=cwd,
> > + environment=STAGING_VARIABLES if get_current_store() ==
> 'staging'
>
> I might be a little off here but if we don't export any variables we are
> by default on production. Only staging parameters need to be exported, if
> that's not the case, can you please show a code example so that I can
> incorporate that in.
>
> > + else None
> > + )
>
>
> --
> https://code.launchpad.net/~canonical-platform-qa/snappy-
> ecosystem-tests/export-staging-variables-ssh/+merge/318743
> You are subscribed to branch lp:snappy-ecosystem-tests.
>

[Heber Parrucci (heber013)]

Reply inline

review: Needs Fixing

[Omer Akram (om26er)]

Heber, replied inline.

[Omer Akram (om26er)]

> There could be situation where container is previously set to staging but
> now needs to be reset to the production (in cases where environment needs
> to be reused).

if get_current_store() returns staging, then staging variables will be available, if it returns 'production' then by default production variables will be exported.

>
> On Fri, Mar 3, 2017 at 4:48 PM, Omer Akram <email address hidden> wrote:
>
> > replied inline.
> >
> > Diff comments:
> >
> > >
> > > === modified file 'snappy_ecosystem_tests/utils/ssh.py'
> > > --- snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > > +++ snappy_ecosystem_tests/utils/ssh.py 2017-03-02 20:39:08 +0000
> > > @@ -31,13 +33,13 @@
> > > class SSHClient(paramiko.SSHClient):
> > > """Extended SSHClient."""
> > >
> > > - def exec_command(self, command, bufsize=-1, timeout=None,
> > get_pty=False,
> > > - cwd=''):
> > > + def exec_command(self, command, bufsize=-1, timeout=None,
> > get_pty=False, # pylint: disable=arguments-differ
> >
> > pylint is picky, yes. But since I am trying to extend the functionality of
> > a method, I added a new parameter, quite normal.
> >
> > pylint threw that warning because it could easily be a mistake on my end,
> > but given its intentional, I had to silence pylint for that. The reason I
> > added that override was inspired by the conversation I found on pylint'
> > mailing list[1].
> >
> > [1] http://code.activestate.com/lists/python-list/188090/
> >
> > > + environment=None, cwd=''):
> > > """Execute the given command over ssh."""
> > > if cwd:
> > > command = 'cd {}; {}'.format(cwd, command)
> > > _, stdout, stderr = super().exec_command(command, bufsize,
> > timeout,
> > > - get_pty)
> > > + get_pty, environment)
> > > if stdout.channel.recv_exit_status() != 0:
> > > raise ValueError(stderr.read().decode().strip())
> > > response = stdout.read().decode().strip()
> > > @@ -118,4 +120,9 @@
> > > _hostname, _username, _port = get_snapd_remote_host_credentials()
> > > ssh = SSHManager.get_instance(
> > > hostname or _hostname, username or _username, port or _port)
> > > - return ssh.exec_command(command, cwd=cwd)
> > > + return ssh.exec_command(
> > > + command,
> > > + cwd=cwd,
> > > + environment=STAGING_VARIABLES if get_current_store() ==
> > 'staging'
> >
> > I might be a little off here but if we don't export any variables we are
> > by default on production. Only staging parameters need to be exported, if
> > that's not the case, can you please show a code example so that I can
> > incorporate that in.
> >
> > > + else None
> > > + )
> >
> >
> > --
> > https://code.launchpad.net/~canonical-platform-qa/snappy-
> > ecosystem-tests/export-staging-variables-ssh/+merge/318743
> > You are subscribed to branch lp:snappy-ecosystem-tests.
> >

[lp:~canonical-platform-qa/snappy-ecosystem-tests/export-staging-variables-ssh]

31. By Omer Akram on 2017-03-03

merge with trunk

[Heber Parrucci (heber013)]

reply inline

review: Abstain

[platform-qa-bot (platform-qa-bot)]

PASSED: Continuous integration, rev:31
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/239/
Executed test runs:
    None: https://platform-qa-jenkins.ubuntu.com/job/generic-update-mp/2044/console

Click here to trigger a rebuild:
https://platform-qa-jenkins.ubuntu.com/job/snappy-ecosystem-tests-ci/239/rebuild

review: Approve (continuous-integration)

[Heber Parrucci (heber013)]

Reply inline

[platform-qa-bot (platform-qa-bot)]

review: Approve (continuous-integration)

=== modified file 'snappy_ecosystem_tests/environment/data/snapd.py'
--- snappy_ecosystem_tests/environment/data/snapd.py	2017-03-01 10:28:47 +0000
+++ snappy_ecosystem_tests/environment/data/snapd.py	2017-03-03 13:01:15 +0000
@@ -67,15 +67,24 @@
     {'command': COMMAND_APT_INSTALL_STAGING_DEBS, 'cwd': DIRECTORY_CLONE}
 ]
 
-CONTAINER_ENV_VARS = {
-    'GOPATH': CONTAINER_HOME,
-    'SNAPPY_USE_STAGING_STORE': '1',
+SNAPCRAFT_STAGING_VARIABLES = {
     ENV_STORE_API_ROOT: URL_API_ROOT,
     ENV_STORE_SEARCH_ROOT: URL_SEARCH_ROOT,
     ENV_STORE_UPLOAD_ROOT: URL_UPLOAD_ROOT,
     ENV_SSO_API_ROOT: URL_SSO_API_ROOT,
 }
 
+SNAPD_STAGING_VARIABLES = {
+    'SNAPPY_USE_STAGING_STORE': '1'
+}
+
+STAGING_VARIABLES = dict(
+    SNAPCRAFT_STAGING_VARIABLES, **SNAPD_STAGING_VARIABLES)
+
+CONTAINER_ENV_VARS = {
+    'GOPATH': CONTAINER_HOME,
+}
+
 DEFAULT_CONTAINER_CONFIG = {
     "name": "snapd",
     "source": {

=== modified file 'snappy_ecosystem_tests/utils/ssh.py'
--- snappy_ecosystem_tests/utils/ssh.py	2017-03-02 18:36:24 +0000
+++ snappy_ecosystem_tests/utils/ssh.py	2017-03-03 13:01:15 +0000
@@ -24,6 +24,8 @@
 import paramiko
 
 from snappy_ecosystem_tests.utils.user import get_snapd_remote_host_credentials
+from snappy_ecosystem_tests.utils.storeconfig import get_current_store
+from snappy_ecosystem_tests.environment.data.snapd import STAGING_VARIABLES
 
 LOGGER = logging.getLogger(__name__)
 
@@ -31,13 +33,13 @@
 class SSHClient(paramiko.SSHClient):
     """Extended SSHClient."""
 
-    def exec_command(self, command, bufsize=-1, timeout=None, get_pty=False,
-                     cwd=''):
+    def exec_command(self, command, bufsize=-1, timeout=None, get_pty=False,  # pylint: disable=arguments-differ
+                     environment=None, cwd=''):
         """Execute the given command over ssh."""
         if cwd:
             command = 'cd {}; {}'.format(cwd, command)
         _, stdout, stderr = super().exec_command(command, bufsize, timeout,
-                                                 get_pty)
+                                                 get_pty, environment)
         if stdout.channel.recv_exit_status() != 0:
             raise ValueError(stderr.read().decode().strip())
         response = stdout.read().decode().strip()
@@ -118,4 +120,9 @@
     _hostname, _username, _port = get_snapd_remote_host_credentials()
     ssh = SSHManager.get_instance(
         hostname or _hostname, username or _username, port or _port)
-    return ssh.exec_command(command, cwd=cwd)
+    return ssh.exec_command(
+        command,
+        cwd=cwd,
+        environment=STAGING_VARIABLES if get_current_store() == 'staging'
+        else None
+    )