Merge lp://staging/~gary/juju-gui/bug1218924 into lp://staging/juju-gui/experimental

Proposed by Gary Poster
Status: Merged
Merged at revision: 1133
Proposed branch: lp://staging/~gary/juju-gui/bug1218924
Merge into: lp://staging/juju-gui/experimental
Diff against target: 57 lines (+30/-5)
2 files modified
Makefile (+2/-0)
docs/process.rst (+28/-5)
To merge this branch: bzr merge lp://staging/~gary/juju-gui/bug1218924
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+190780@code.staging.launchpad.net

Description of the change

Change distfile to exclude unnecessary bits

In order to address the charm deployment problems reported today, this is a first step of two that significantly shrinks the size of our releases. It takes our release down from about 43M to 9M; that brings our charm from about 45M to 11M. I'll make another step (switch from gz to xz) that can bring us down another couple of M. Hopefully that will be sufficient for the problem.

The code is pretty easy to review, but the qa is a bit more tedious. First, please make a distfile (BRANCH_IS_GOOD=1 make distfile) and then follow the instructions I added to the release instructions to test the resulting tarball. Second, add the tarball to a local copy of the charm in the releases directory, remove the 0.10.1 tarball, juju bootstrap, and run make deploy in the charm. Once the charm is deployed, you should be able to see qa the site.

https://codereview.appspot.com/14516054/

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :
Download full text (4.1 KiB)

Reviewers: mp+190780_code.launchpad.net,

Message:
Please take a look.

Description:
Change distfile to exclude unnecessary bits

In order to address the charm deployment problems reported today, this
is a first step of two that significantly shrinks the size of our
releases. It takes our release down from about 43M to 9M; that brings
our charm from about 45M to 11M. I'll make another step (switch from gz
to xz) that can bring us down another couple of M. Hopefully that will
be sufficient for the problem.

The code is pretty easy to review, but the qa is a bit more tedious.
First, please make a distfile (BRANCH_IS_GOOD=1 make distfile) and then
follow the instructions I added to the release instructions to test the
resulting tarball. Second, add the tarball to a local copy of the charm
in the releases directory, remove the 0.10.1 tarball, juju bootstrap,
and run make deploy in the charm. Once the charm is deployed, you
should be able to see qa the site.

https://code.launchpad.net/~gary/juju-gui/bug1218924/+merge/190780

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14516054/

Affected files (+32, -5 lines):
   M Makefile
   A [revision details]
   M docs/process.rst

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2013-09-04 10:48:56 +0000
+++ Makefile 2013-10-12 01:30:26 +0000
@@ -551,6 +551,8 @@
   mkdir -p releases
   # When creating the tarball, ensure all symbolic links are followed.
   tar -c --auto-compress --exclude-vcs --exclude releases \
+ --exclude node_modules --exclude virtualenv --exclude app \
+ --exclude bin --exclude archives --exclude build-shared \
       --dereference --transform "s|^|$(RELEASE_NAME)/|" -f $(RELEASE_FILE) *
   @echo "Release was created in $(RELEASE_FILE)."
  else

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: docs/process.rst
=== modified file 'docs/process.rst'
--- docs/process.rst 2013-10-11 11:04:36 +0000
+++ docs/process.rst 2013-10-12 01:53:31 +0000
@@ -166,12 +166,35 @@
  - Ensure that the ``build-prod/juju-ui/version.js`` file contains a version
    string that combines the value in the branch's ``CHANGES.yaml`` with the
    branch's revno.
-- Start the ``improv.py`` script as described in the HACKING file.
  - While still in the directory where you extracted the tar file, change
- app/config-prod.js to specify apiBackend: 'python'.
-- While still in the directory where you extracted the tar file, run the
- command: ``NO_BZR=1 make prod``.
-- Go to the URL shown in the terminal.
+ build-prod/juju-ui/assets/config.js to specify sandbox: true,
+ defaultViewmode: 'fullscreen', user: 'admin', password: 'admin',
+ simulateEvents: false, isJujucharms: true, and showGetJujuButton: true.
+- Configure a webserver to serve the files, if you have not already. For
+ example, these are nginx instructions.
+
+ - ``sudo apt-get install nginx``
...

Read more...

Revision history for this message
Francesco Banconi (frankban) wrote :

This branch is very nice Gary, thank you!
LGTM and QA ok (both local nginx and the deployed charm with sandbox
mode on and off). I only have a consideration about isJujucharm, please
read below.

https://codereview.appspot.com/14516054/diff/1/docs/process.rst
File docs/process.rst (right):

https://codereview.appspot.com/14516054/diff/1/docs/process.rst#newcode172
docs/process.rst:172: simulateEvents: false, isJujucharms: true, and
showGetJujuButton: true.
After setting isJujucharm to false the GUI seems broken: the charm
browser is replaced by a "Hello, world!" and therefore cannot be open.
This is not a bug of this branch, this happens also when using "make
debug" with isJujucharm=true. Setting it to false or navigating, e.g. to
"/sidebar" fixes the problem. I'd suggest to leave isJujucharms
untouched in this QA instructions.

Minor minor: simulateEvents is already set to false.

https://codereview.appspot.com/14516054/

1133. By Gary Poster

respond to review

Revision history for this message
Gary Poster (gary) wrote :

*** Submitted:

Change distfile to exclude unnecessary bits

In order to address the charm deployment problems reported today, this
is a first step of two that significantly shrinks the size of our
releases. It takes our release down from about 43M to 9M; that brings
our charm from about 45M to 11M. I'll make another step (switch from gz
to xz) that can bring us down another couple of M. Hopefully that will
be sufficient for the problem.

The code is pretty easy to review, but the qa is a bit more tedious.
First, please make a distfile (BRANCH_IS_GOOD=1 make distfile) and then
follow the instructions I added to the release instructions to test the
resulting tarball. Second, add the tarball to a local copy of the charm
in the releases directory, remove the 0.10.1 tarball, juju bootstrap,
and run make deploy in the charm. Once the charm is deployed, you
should be able to see qa the site.

R=frankban
CC=
https://codereview.appspot.com/14516054

https://codereview.appspot.com/14516054/

Revision history for this message
Gary Poster (gary) wrote :

On 2013/10/14 09:31:41, frankban wrote:
> This branch is very nice Gary, thank you!
> LGTM and QA ok (both local nginx and the deployed charm with sandbox
mode on and
> off). I only have a consideration about isJujucharm, please read
below.

> https://codereview.appspot.com/14516054/diff/1/docs/process.rst
> File docs/process.rst (right):

https://codereview.appspot.com/14516054/diff/1/docs/process.rst#newcode172
> docs/process.rst:172: simulateEvents: false, isJujucharms: true, and
> showGetJujuButton: true.
> After setting isJujucharm to false the GUI seems broken: the charm
browser is
> replaced by a "Hello, world!" and therefore cannot be open.
> This is not a bug of this branch, this happens also when using "make
debug" with
> isJujucharm=true. Setting it to false or navigating, e.g. to
"/sidebar" fixes
> the problem. I'd suggest to leave isJujucharms untouched in this QA
> instructions.

> Minor minor: simulateEvents is already set to false.

Thank you very much for the review and qa, Francesco. I removed the
instructions referencing the isJujucharm flag and filed
https://bugs.launchpad.net/juju-gui/+bug/1239671 to remove the flag
itself eventually. I decided to keep mentioning the simulateEvents
because I wanted people to verify that they were testing with the
simulation turned off.

Gary

https://codereview.appspot.com/14516054/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches