Merge lp:~ce-infrastructure/capomastro/capomastro-jenkins-exposure-1155 into lp:capomastro

[Caio Begotti]

This is part of a task to story 1155 about investigating whether we need to expose Jenkins for real or not. If this merge is accepted I think it's fairly safe to commit and push directly the following change to the deploy branch of Capomastro, too:

=== modified file 'deploy.sh'
--- deploy.sh 2014-11-13 02:51:13 +0000
+++ deploy.sh 2014-11-19 21:52:22 +0000
@@ -43,5 +43,4 @@
 juju add-relation apache2:reverseproxy capomastro:website && sleep 30

 # Expose web services
-juju expose jenkins
 juju expose apache2

The tl;dr of these commits is: we don't need to overexpose Jenkins really, but some warning to the users about not having an archiver setup is good since the archiver is the one which would replace the Jenkins URLs so users can download artifacts.

[Daniel Manrique (roadmr)]

I made an inline comment, plus I notice there are no tests for this and I'm wondering if it's worth somehow testing it. Perhaps checking that the context has no "archive" value, then creating an Archive and checking again, this time that the context has the archive attribute.

I found a possibly related snippet in archives/tests/test_helpers.py, that may prove helpful.

It may also be possible to do a test where we actually verify that the rendered template has the warning string if there's no archive.

Apologies if this is vague, it's just showing my incomplete knowledge of django templates and testing :/

review: Needs Information

[Sheila Miguez (codersquid)]

I added a diff command about using "include" as an option.

[Caio Begotti (caio1982)]

Thanks for the Django tips, I didn't know how we could tests a template but
I'll take a look at it and use a small template just for this alert then.

On Thu, Nov 20, 2014 at 8:23 PM, Sheila Miguez <email address hidden>
wrote:

> I added a diff command about using "include" as an option.
>
> Diff comments:
>
> > === modified file 'capomastro/local_settings.py'
> > --- capomastro/local_settings.py 2014-11-03 19:06:32 +0000
> > +++ capomastro/local_settings.py 2014-11-20 20:20:15 +0000
> > @@ -1,6 +1,5 @@
> > # SECURITY WARNING: don't run with debug turned on in production!
> > -DEBUG = False
> > -
> > +DEBUG = True
> > TEMPLATE_DEBUG = True
> > ALLOWED_HOSTS = []
> >
> >
> > === modified file 'debian/changelog'
> > --- debian/changelog 2014-11-03 19:06:32 +0000
> > +++ debian/changelog 2014-11-20 20:20:15 +0000
> > @@ -1,3 +1,10 @@
> > +capomastro (20141120) trusty; urgency=high
> > +
> > + * Adds warning to avoid overexposing the Jenkins server.
> > + * Adds two testing scripts to use with LXC and Vagrant.
> > +
> > + -- Caio Begotti <email address hidden> Thu, 20 Nov 2014
> 10:41:23 -0200
> > +
> > capomastro (20141103) trusty; urgency=high
> >
> > * Partially fixes https://bugs.launchpad.net/capomastro/+bug/1388936
> >
> > === modified file 'projects/templates/projects/dependency_detail.html'
> > --- projects/templates/projects/dependency_detail.html 2014-07-18
> 12:07:38 +0000
> > +++ projects/templates/projects/dependency_detail.html 2014-11-20
> 20:20:15 +0000
> > @@ -11,6 +11,9 @@
> > <div class="container">
> > <div class="row">
> > <div>
> > + {% if not archiver %}
> > + {% bootstrap_alert "Could not find a default archiver in the
> configuration. <a href='/admin/archives/archive/'>Please set up one</a>,
> otherwise artifacts of this dependency will only be available through an
> exposed Jenkins server." alert_type='warning' %}
> > + {% endif %}
>
> One way to handle it is to use an include.
>
> {% include "archiver_alert.html" %}
>
> you can also pass objects to it in case you want to tweak the snippet, for
> example
>
> {% include "archiver_alert.html" with somevar="foobar"%}
>
> > <h2>{{ dependency.name }}</h2>
> > {% if dependency.get_build_parameters %}
> > <table class="table table-striped">
> >
> > === modified file 'projects/templates/projects/dependency_form.html'
> > --- projects/templates/projects/dependency_form.html 2014-06-02
> 10:55:15 +0000
> > +++ projects/templates/projects/dependency_form.html 2014-11-20
> 20:20:15 +0000
> > @@ -8,6 +8,9 @@
> > <div class="container">
> > <div class="row">
> > <div>
> > + {% if not archiver %}
> > + {% bootstrap_alert "Could not find a default archiver in the
> configuration. <a href='/admin/archives/archive/'>Please set up one</a>,
> otherwise artifacts of this dependency will only be available through an
> exposed Jenkins server." alert_type='warning' %}
> > + {% endif %}
> > <h2>Dependency</h2>
> > <form action="" method="post" class="form" id="dependency">
> > {% csrf_token %}
> >
> > === modified file 'projects/templates/projects/projec...

[lp:~ce-infrastructure/capomastro/capomastro-jenkins-exposure-1155]

150. By Caio Begotti on 2014-11-21

moving all these alerts to a specific template should have been done in the first place i guess

151. By Caio Begotti on 2014-11-21

dunno why django ignores the none value of get_default_archive when passing it to the template's context, thus here it's simply assertingequal to '' instead

152. By Caio Begotti on 2014-11-21

we better not mess with the form submission so it is now tested before the submit happens just in case we want/need to change the form tests later so there is no need to change the archiver ones too

153. By Caio Begotti on 2014-11-21

trailing space leftover

154. By Caio Begotti on 2014-11-21

yet another trailing space leftover

[Caio Begotti (caio1982)]

Hey, what do you guys think about it now? I figure that we should not (and are not) test visual elements in the views (i.e. the bootstrap alert) but simply checking if the context has the archiver available for the test is fair enough. I thought the contexts checks would be duplicated but that's how the other tests are doing it so no need to worry about it I think.

[Daniel Manrique (roadmr)]

Looks good to me, the extracted template looks much neater, and thanks for adding the tests. I agree that for something this simple it's OK to just check the context for the archiver data, not validating the actual rendering since it's just a static chunk with no extra functionality.

review: Approve

[Sheila Miguez (codersquid)]

I haven't set up tests locally to see if I could help with understanding why the context is populated with archiver. I would have expected the test to blow up because of it not being a key.

if it shouldn't be there, I would assert that archiver is not in the keys. I am okay with committing this anyway even though the test is weird. None or '' is falsy and the template shouldn't barf.

review: Approve

=== modified file 'capomastro/local_settings.py'
--- capomastro/local_settings.py	2014-11-03 19:06:32 +0000
+++ capomastro/local_settings.py	2014-11-21 12:19:03 +0000
@@ -1,6 +1,5 @@
 # SECURITY WARNING: don't run with debug turned on in production!
-DEBUG = False
-
+DEBUG = True
 TEMPLATE_DEBUG = True
 ALLOWED_HOSTS = []
 

=== modified file 'debian/changelog'
--- debian/changelog	2014-11-03 19:06:32 +0000
+++ debian/changelog	2014-11-21 12:19:03 +0000
@@ -1,3 +1,10 @@
+capomastro (20141120) trusty; urgency=high
+
+  * Adds warning to avoid overexposing the Jenkins server.
+  * Adds two testing scripts to use with LXC and Vagrant.
+
+ -- Caio Begotti <[e-mail]>  Thu, 20 Nov 2014 10:41:23 -0200
+
 capomastro (20141103) trusty; urgency=high
 
   * Partially fixes https://bugs.launchpad.net/capomastro/+bug/1388936

=== modified file 'projects/templates/projects/dependency_detail.html'
--- projects/templates/projects/dependency_detail.html	2014-07-18 12:07:38 +0000
+++ projects/templates/projects/dependency_detail.html	2014-11-21 12:19:03 +0000
@@ -11,6 +11,7 @@
 <div class="container">
   <div class="row">
     <div>
+      {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
       <h2>{{ dependency.name }}</h2>
       {% if dependency.get_build_parameters %}
       <table class="table table-striped">

=== modified file 'projects/templates/projects/dependency_form.html'
--- projects/templates/projects/dependency_form.html	2014-06-02 10:55:15 +0000
+++ projects/templates/projects/dependency_form.html	2014-11-21 12:19:03 +0000
@@ -8,6 +8,7 @@
 <div class="container">
   <div class="row">
     <div>
+      {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
       <h2>Dependency</h2>
       <form action="" method="post" class="form" id="dependency">
         {% csrf_token %}

=== added file 'projects/templates/projects/missing_archiver_alert.html'
--- projects/templates/projects/missing_archiver_alert.html	1970-01-01 00:00:00 +0000
+++ projects/templates/projects/missing_archiver_alert.html	2014-11-21 12:19:03 +0000
@@ -0,0 +1,4 @@
+{% load bootstrap3 %}
+{% if not archiver %}
+{% bootstrap_alert "Could not find a default archiver in the configuration. <a href='/admin/archives/archive/'>Please set up one</a>, otherwise built artifacts will only be available through an exposed Jenkins server." alert_type='warning' %}
+{% endif %}

=== modified file 'projects/templates/projects/project_detail.html'
--- projects/templates/projects/project_detail.html	2014-07-25 11:37:42 +0000
+++ projects/templates/projects/project_detail.html	2014-11-21 12:19:03 +0000
@@ -8,6 +8,7 @@
 <div class="container">
   <div class="row">
     <div>
+      {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
       <h2>{{ project.name }}</h2>
     </div>
   </div>

=== modified file 'projects/templates/projects/project_form.html'
--- projects/templates/projects/project_form.html	2014-03-20 16:17:58 +0000
+++ projects/templates/projects/project_form.html	2014-11-21 12:19:03 +0000
@@ -8,6 +8,7 @@
 <div class="container">
   <div class="row">
     <div>
+      {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
       <h2>Project</h2>
       <form action="" method="post" class="form" id="project">
         {% csrf_token %}

=== modified file 'projects/templates/projects/projectbuild_detail.html'
--- projects/templates/projects/projectbuild_detail.html	2014-07-25 11:37:42 +0000
+++ projects/templates/projects/projectbuild_detail.html	2014-11-21 12:19:03 +0000
@@ -31,7 +31,7 @@
           <td>{{ dependency.build.number }}</td>
           <td>{{ dependency.dependency.name }}</td>
           <td>{{ dependency.build.status }}</td>
-          <td><a href="{{ dependency.build.url }}">{{ dependency.build.phase }}</a></td>
+          <td>{{ dependency.build.phase }}</td>
           {% else %}
           <td colspan="4">Waiting for build of job {{ dependency.job }}</td>
           {% endif %}

=== modified file 'projects/tests/test_views.py'
--- projects/tests/test_views.py	2014-07-25 14:58:58 +0000
+++ projects/tests/test_views.py	2014-11-21 12:19:03 +0000
@@ -54,6 +54,11 @@
         self.assertEqual(project, response.context["project"])
         self.assertEqual([dependency], list(response.context["dependencies"]))
 
+        self.assertEqual('', response.context["archiver"])
+        archive = ArchiveFactory.create(policy="cdimage", default=True)
+        response = self.app.get(project_url, user="testing")
+        self.assertEqual(archive, response.context["archiver"])
+
         self.assertEqual(
             sorted(projectbuilds[1:], key=lambda x: x.build_id, reverse=True),
             list(response.context["projectbuilds"]))
@@ -158,6 +163,12 @@
         """
         project_url = reverse("project_create")
         response = self.app.get(project_url, user="testing")
+
+        self.assertEqual('', response.context["archiver"])
+        archive = ArchiveFactory.create(policy="cdimage", default=True)
+        response = self.app.get(project_url, user="testing")
+        self.assertEqual(archive, response.context["archiver"])
+ 
         form = response.forms["project"]
         self.assertTrue(
             form["auto_track"].value, "auto_track not defaulting to True")
@@ -367,6 +378,11 @@
         project_url = reverse("dependency_create")
         response = self.app.get(project_url, user="testing")
 
+        self.assertEqual('', response.context["archiver"])
+        archive = ArchiveFactory.create(policy="cdimage", default=True)
+        response = self.app.get(project_url, user="testing")
+        self.assertEqual(archive, response.context["archiver"])
+
         form = response.forms["dependency"]
         form["jobtype"].select(self.jobtype.pk)
         form["server"].select(self.server.pk)
@@ -426,6 +442,11 @@
         self.assertEqual([project], list(response.context["projects"]))
         self.assertNotContains(response, "Dependency currently building")
 
+        self.assertEqual('', response.context["archiver"])
+        archive = ArchiveFactory.create(policy="cdimage", default=True)
+        response = self.app.get(url, user="testing")
+        self.assertEqual(archive, response.context["archiver"])
+
     def test_dependency_detail_with_currently_building(self):
         """
         If the Dependency is currently building, we should get an info message

=== modified file 'projects/views.py'
--- projects/views.py	2014-07-25 14:58:58 +0000
+++ projects/views.py	2014-11-21 12:19:03 +0000
@@ -46,6 +46,19 @@
         initial["auto_track"] = True
         return initial
 
+    def get_context_data(self, **kwargs):
+        """
+        Supplement the project view with an archive check
+        """
+        context = super(
+            ProjectCreateView, self).get_context_data(**kwargs)
+
+        archive = get_default_archive()
+        if archive:
+            context["archiver"] = archive
+
+        return context
+
 
 class ProjectListView(LoginRequiredMixin, ListView):
 
@@ -194,6 +207,11 @@
                 for artifact in current_projectbuild.get_current_artifacts():
                     items.append(self.item_from_artifact(artifact))
         context["current_artifacts"] = items
+
+        archive = get_default_archive()
+        if archive:
+            context["archiver"] = archive
+
         return context
 
     @staticmethod
@@ -237,6 +255,20 @@
         url_args = {"pk": self.object.pk}
         return reverse("dependency_detail", kwargs=url_args)
 
+    def get_context_data(self, **kwargs):
+        """
+        Supplement the dependency view with an archive check
+        """
+        context = super(
+            DependencyCreateView, self).get_context_data(**kwargs)
+
+        archive = get_default_archive()
+        if archive:
+            context["archiver"] = archive
+
+        return context
+
+
 
 class DependencyListView(LoginRequiredMixin, ListView):
 
@@ -285,6 +317,11 @@
             messages.add_message(
                 self.request, messages.INFO,
                 "Dependency currently building")
+
+        archive = get_default_archive()
+        if archive:
+            context["archiver"] = archive
+
         return context
 
     def post(self, request, pk):