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

Proposed by Caio Begotti on 2014-11-20
Status: Merged
Approved by: Daniel Manrique on 2014-11-21
Approved revision: 154
Merged at revision: 141
Proposed branch: lp:~ce-infrastructure/capomastro/capomastro-jenkins-exposure-1155
Merge into: lp:capomastro
Diff against target: 218 lines (+75/-3)
10 files modified
capomastro/local_settings.py (+1/-2)
debian/changelog (+7/-0)
projects/templates/projects/dependency_detail.html (+1/-0)
projects/templates/projects/dependency_form.html (+1/-0)
projects/templates/projects/missing_archiver_alert.html (+4/-0)
projects/templates/projects/project_detail.html (+1/-0)
projects/templates/projects/project_form.html (+1/-0)
projects/templates/projects/projectbuild_detail.html (+1/-1)
projects/tests/test_views.py (+21/-0)
projects/views.py (+37/-0)
To merge this branch: bzr merge lp:~ce-infrastructure/capomastro/capomastro-jenkins-exposure-1155
Reviewer Review Type Date Requested Status
Sheila Miguez (community) 2014-11-21 Approve on 2014-11-21
Daniel Manrique 2014-11-20 Approve on 2014-11-21
Review via email: mp+242412@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Caio Begotti (caio1982) :
Daniel Manrique (roadmr) wrote :

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) wrote :

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

Caio Begotti (caio1982) wrote :
Download full text (7.5 KiB)

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

Read more...

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) wrote :

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.

Caio Begotti (caio1982) :
Daniel Manrique (roadmr) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'capomastro/local_settings.py'
2--- capomastro/local_settings.py 2014-11-03 19:06:32 +0000
3+++ capomastro/local_settings.py 2014-11-21 12:19:03 +0000
4@@ -1,6 +1,5 @@
5 # SECURITY WARNING: don't run with debug turned on in production!
6-DEBUG = False
7-
8+DEBUG = True
9 TEMPLATE_DEBUG = True
10 ALLOWED_HOSTS = []
11
12
13=== modified file 'debian/changelog'
14--- debian/changelog 2014-11-03 19:06:32 +0000
15+++ debian/changelog 2014-11-21 12:19:03 +0000
16@@ -1,3 +1,10 @@
17+capomastro (20141120) trusty; urgency=high
18+
19+ * Adds warning to avoid overexposing the Jenkins server.
20+ * Adds two testing scripts to use with LXC and Vagrant.
21+
22+ -- Caio Begotti <caio.begotti@canonical.com> Thu, 20 Nov 2014 10:41:23 -0200
23+
24 capomastro (20141103) trusty; urgency=high
25
26 * Partially fixes https://bugs.launchpad.net/capomastro/+bug/1388936
27
28=== modified file 'projects/templates/projects/dependency_detail.html'
29--- projects/templates/projects/dependency_detail.html 2014-07-18 12:07:38 +0000
30+++ projects/templates/projects/dependency_detail.html 2014-11-21 12:19:03 +0000
31@@ -11,6 +11,7 @@
32 <div class="container">
33 <div class="row">
34 <div>
35+ {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
36 <h2>{{ dependency.name }}</h2>
37 {% if dependency.get_build_parameters %}
38 <table class="table table-striped">
39
40=== modified file 'projects/templates/projects/dependency_form.html'
41--- projects/templates/projects/dependency_form.html 2014-06-02 10:55:15 +0000
42+++ projects/templates/projects/dependency_form.html 2014-11-21 12:19:03 +0000
43@@ -8,6 +8,7 @@
44 <div class="container">
45 <div class="row">
46 <div>
47+ {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
48 <h2>Dependency</h2>
49 <form action="" method="post" class="form" id="dependency">
50 {% csrf_token %}
51
52=== added file 'projects/templates/projects/missing_archiver_alert.html'
53--- projects/templates/projects/missing_archiver_alert.html 1970-01-01 00:00:00 +0000
54+++ projects/templates/projects/missing_archiver_alert.html 2014-11-21 12:19:03 +0000
55@@ -0,0 +1,4 @@
56+{% load bootstrap3 %}
57+{% if not archiver %}
58+{% 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' %}
59+{% endif %}
60
61=== modified file 'projects/templates/projects/project_detail.html'
62--- projects/templates/projects/project_detail.html 2014-07-25 11:37:42 +0000
63+++ projects/templates/projects/project_detail.html 2014-11-21 12:19:03 +0000
64@@ -8,6 +8,7 @@
65 <div class="container">
66 <div class="row">
67 <div>
68+ {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
69 <h2>{{ project.name }}</h2>
70 </div>
71 </div>
72
73=== modified file 'projects/templates/projects/project_form.html'
74--- projects/templates/projects/project_form.html 2014-03-20 16:17:58 +0000
75+++ projects/templates/projects/project_form.html 2014-11-21 12:19:03 +0000
76@@ -8,6 +8,7 @@
77 <div class="container">
78 <div class="row">
79 <div>
80+ {% include "projects/missing_archiver_alert.html" with archiver=archiver %}
81 <h2>Project</h2>
82 <form action="" method="post" class="form" id="project">
83 {% csrf_token %}
84
85=== modified file 'projects/templates/projects/projectbuild_detail.html'
86--- projects/templates/projects/projectbuild_detail.html 2014-07-25 11:37:42 +0000
87+++ projects/templates/projects/projectbuild_detail.html 2014-11-21 12:19:03 +0000
88@@ -31,7 +31,7 @@
89 <td>{{ dependency.build.number }}</td>
90 <td>{{ dependency.dependency.name }}</td>
91 <td>{{ dependency.build.status }}</td>
92- <td><a href="{{ dependency.build.url }}">{{ dependency.build.phase }}</a></td>
93+ <td>{{ dependency.build.phase }}</td>
94 {% else %}
95 <td colspan="4">Waiting for build of job {{ dependency.job }}</td>
96 {% endif %}
97
98=== modified file 'projects/tests/test_views.py'
99--- projects/tests/test_views.py 2014-07-25 14:58:58 +0000
100+++ projects/tests/test_views.py 2014-11-21 12:19:03 +0000
101@@ -54,6 +54,11 @@
102 self.assertEqual(project, response.context["project"])
103 self.assertEqual([dependency], list(response.context["dependencies"]))
104
105+ self.assertEqual('', response.context["archiver"])
106+ archive = ArchiveFactory.create(policy="cdimage", default=True)
107+ response = self.app.get(project_url, user="testing")
108+ self.assertEqual(archive, response.context["archiver"])
109+
110 self.assertEqual(
111 sorted(projectbuilds[1:], key=lambda x: x.build_id, reverse=True),
112 list(response.context["projectbuilds"]))
113@@ -158,6 +163,12 @@
114 """
115 project_url = reverse("project_create")
116 response = self.app.get(project_url, user="testing")
117+
118+ self.assertEqual('', response.context["archiver"])
119+ archive = ArchiveFactory.create(policy="cdimage", default=True)
120+ response = self.app.get(project_url, user="testing")
121+ self.assertEqual(archive, response.context["archiver"])
122+
123 form = response.forms["project"]
124 self.assertTrue(
125 form["auto_track"].value, "auto_track not defaulting to True")
126@@ -367,6 +378,11 @@
127 project_url = reverse("dependency_create")
128 response = self.app.get(project_url, user="testing")
129
130+ self.assertEqual('', response.context["archiver"])
131+ archive = ArchiveFactory.create(policy="cdimage", default=True)
132+ response = self.app.get(project_url, user="testing")
133+ self.assertEqual(archive, response.context["archiver"])
134+
135 form = response.forms["dependency"]
136 form["jobtype"].select(self.jobtype.pk)
137 form["server"].select(self.server.pk)
138@@ -426,6 +442,11 @@
139 self.assertEqual([project], list(response.context["projects"]))
140 self.assertNotContains(response, "Dependency currently building")
141
142+ self.assertEqual('', response.context["archiver"])
143+ archive = ArchiveFactory.create(policy="cdimage", default=True)
144+ response = self.app.get(url, user="testing")
145+ self.assertEqual(archive, response.context["archiver"])
146+
147 def test_dependency_detail_with_currently_building(self):
148 """
149 If the Dependency is currently building, we should get an info message
150
151=== modified file 'projects/views.py'
152--- projects/views.py 2014-07-25 14:58:58 +0000
153+++ projects/views.py 2014-11-21 12:19:03 +0000
154@@ -46,6 +46,19 @@
155 initial["auto_track"] = True
156 return initial
157
158+ def get_context_data(self, **kwargs):
159+ """
160+ Supplement the project view with an archive check
161+ """
162+ context = super(
163+ ProjectCreateView, self).get_context_data(**kwargs)
164+
165+ archive = get_default_archive()
166+ if archive:
167+ context["archiver"] = archive
168+
169+ return context
170+
171
172 class ProjectListView(LoginRequiredMixin, ListView):
173
174@@ -194,6 +207,11 @@
175 for artifact in current_projectbuild.get_current_artifacts():
176 items.append(self.item_from_artifact(artifact))
177 context["current_artifacts"] = items
178+
179+ archive = get_default_archive()
180+ if archive:
181+ context["archiver"] = archive
182+
183 return context
184
185 @staticmethod
186@@ -237,6 +255,20 @@
187 url_args = {"pk": self.object.pk}
188 return reverse("dependency_detail", kwargs=url_args)
189
190+ def get_context_data(self, **kwargs):
191+ """
192+ Supplement the dependency view with an archive check
193+ """
194+ context = super(
195+ DependencyCreateView, self).get_context_data(**kwargs)
196+
197+ archive = get_default_archive()
198+ if archive:
199+ context["archiver"] = archive
200+
201+ return context
202+
203+
204
205 class DependencyListView(LoginRequiredMixin, ListView):
206
207@@ -285,6 +317,11 @@
208 messages.add_message(
209 self.request, messages.INFO,
210 "Dependency currently building")
211+
212+ archive = get_default_archive()
213+ if archive:
214+ context["archiver"] = archive
215+
216 return context
217
218 def post(self, request, pk):

Subscribers

People subscribed via source and target branches