Merge lp://qastaging/~frankban/juju-quickstart/new-auth-api-endpoint into lp://qastaging/juju-quickstart

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 121
Proposed branch: lp://qastaging/~frankban/juju-quickstart/new-auth-api-endpoint
Merge into: lp://qastaging/juju-quickstart
Diff against target: 1044 lines (+441/-91)
12 files modified
quickstart/app.py (+36/-19)
quickstart/jujutools.py (+42/-1)
quickstart/manage.py (+19/-7)
quickstart/models/charms.py (+4/-0)
quickstart/models/jenv.py (+17/-0)
quickstart/settings.py (+8/-2)
quickstart/tests/helpers.py (+1/-0)
quickstart/tests/models/test_charms.py (+28/-0)
quickstart/tests/models/test_jenv.py (+24/-0)
quickstart/tests/test_app.py (+79/-44)
quickstart/tests/test_jujutools.py (+118/-0)
quickstart/tests/test_manage.py (+65/-18)
To merge this branch: bzr merge lp://qastaging/~frankban/juju-quickstart/new-auth-api-endpoint
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+249102@code.qastaging.launchpad.net

Description of the change

Add support for new Juju WebSocket API endpoints.

Recent Juju versions introduced a new API endpoint
path. In essence, instead of the usual
"wss://<address>:17070", the new
"wss://<address>:17070/environment/<env-uuid>/api"
is used to connect to the API.
This allows for connecting to a specific environment
in a multi-environment state server scenario.

In this branch the new API endpoint is used if a recent
Juju version is in use, and if it is possible to retrieve
the environment UUID from the jenv file.

Also, when connecting to the GUI server (for creating
the auth token or for deploying bundles), use the new
GUI server API endpoints when possible, i.e. when the
charm is recent enough to support redirecting requests
to the new Juju endpoints.
Note that this feature is assumed to land in the next
juju-gui charm release (see settings.py). If that's
not the case, we'll need to increase the charm revisions
in settings.MINIMUM_REVISIONS_FOR_NEW_API_ENDPOINT
before releasing the new Quickstart.

Tests: `make check`

QA:
- bootstrap quickstart as usual: `devenv/bin/juju-quickstart`;
- check that, if you are using juju devel (1.22beta), quickstart
  properly connect to the new API endpoint;
- run quickstart again to deploy a bundle, e.g.:
  `devenv/bin/juju-quickstart bundle:mediawiki/single`;
- ensure that the deployment request succeeds;
- if possible, do the above with and older version of Juju,
  to ensure backward compatibility.

Done, thank you!

https://codereview.appspot.com/199490043/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+249102_code.launchpad.net,

Message:
Please take a look.

Description:
Add support for new Juju WebSocket API endpoints.

Recent Juju versions introduced a new API endpoint
path. In essence, instead of the usual
"wss://<address>:17070", the new
"wss://<address>:17070/environment/<env-uuid>/api"
is used to connect to the API.
This allows for connecting to a specific environment
in a multi-environment state server scenario.

In this branch the new API endpoint is used if a recent
Juju version is in use, and if it is possible to retrieve
the environment UUID from the jenv file.

Also, when connecting to the GUI server (for creating
the auth token or for deploying bundles), use the new
GUI server API endpoints when possible, i.e. when the
charm is recent enough to support redirecting requests
to the new Juju endpoints.
Note that this feature is assumed to land in the next
juju-gui charm release (see settings.py). If that's
not the case, we'll need to increase the charm revisions
in settings.MINIMUM_REVISIONS_FOR_NEW_API_ENDPOINT
before releasing the new Quickstart.

Tests: `make check`

QA:
- bootstrap quickstart as usual: `devenv/bin/juju-quickstart`;
- check that, if you are using juju devel (1.22beta), quickstart
   properly connect to the new API endpoint;
- run quickstart again to deploy a bundle, e.g.:
   `devenv/bin/juju-quickstart bundle:mediawiki/single`;
- ensure that the deployment request succeeds;
- if possible, do the above with and older version of Juju,
   to ensure backward compatibility.

Done, thank you!

https://code.launchpad.net/~frankban/juju-quickstart/new-auth-api-endpoint/+merge/249102

(do not edit description out of merge proposal)

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

Affected files (+443, -91 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/jujutools.py
   M quickstart/manage.py
   M quickstart/models/charms.py
   M quickstart/models/jenv.py
   M quickstart/settings.py
   M quickstart/tests/helpers.py
   M quickstart/tests/models/test_charms.py
   M quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_jujutools.py
   M quickstart/tests/test_manage.py

Revision history for this message
Martin Hilton (martin-hilton) wrote :

LGTM: No QA

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py#newcode283
quickstart/app.py:283: def get_env_uuid_or_none(env_name):
Is it really necessary to have or_none in the name of this function?

https://codereview.appspot.com/199490043/

Revision history for this message
Jeff Pihach (hatch) wrote :

LGTM with some possible cleanups.
QA OK!

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py#newcode283
quickstart/app.py:283: def get_env_uuid_or_none(env_name):
On 2015/02/10 10:47:32, martin.hilton wrote:
> Is it really necessary to have or_none in the name of this function?

+1

https://codereview.appspot.com/199490043/diff/1/quickstart/models/jenv.py
File quickstart/models/jenv.py (right):

https://codereview.appspot.com/199490043/diff/1/quickstart/models/jenv.py#newcode134
quickstart/models/jenv.py:134: data =
serializers.yaml_load_from_path(jenv_path)
I believe there are other places in the code which require information
from the jenv file so I figured that this would already be abstracted
out into a utility method already so you could just fetch the value.

https://codereview.appspot.com/199490043/

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

Thanks for the reviews!

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/199490043/diff/1/quickstart/app.py#newcode283
quickstart/app.py:283: def get_env_uuid_or_none(env_name):
On 2015/02/10 10:47:32, martin.hilton wrote:
> Is it really necessary to have or_none in the name of this function?

Not strictly necessary, but in the caller context this can help: while
most of the times app functions return values, this can also return
none, i.e. do not rely on the fact the env uuid is always known.

https://codereview.appspot.com/199490043/diff/1/quickstart/models/jenv.py
File quickstart/models/jenv.py (right):

https://codereview.appspot.com/199490043/diff/1/quickstart/models/jenv.py#newcode134
quickstart/models/jenv.py:134: data =
serializers.yaml_load_from_path(jenv_path)
On 2015/02/10 15:12:09, jeff.pihach wrote:
> I believe there are other places in the code which require information
from the
> jenv file so I figured that this would already be abstracted out into
a utility
> method already so you could just fetch the value.

A slight refactor of the code in the jenv models can help, I agree.
On the other hand, the repeated code is still just two lines, so perhaps
not something for this branch.

https://codereview.appspot.com/199490043/

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

*** Submitted:

Add support for new Juju WebSocket API endpoints.

Recent Juju versions introduced a new API endpoint
path. In essence, instead of the usual
"wss://<address>:17070", the new
"wss://<address>:17070/environment/<env-uuid>/api"
is used to connect to the API.
This allows for connecting to a specific environment
in a multi-environment state server scenario.

In this branch the new API endpoint is used if a recent
Juju version is in use, and if it is possible to retrieve
the environment UUID from the jenv file.

Also, when connecting to the GUI server (for creating
the auth token or for deploying bundles), use the new
GUI server API endpoints when possible, i.e. when the
charm is recent enough to support redirecting requests
to the new Juju endpoints.
Note that this feature is assumed to land in the next
juju-gui charm release (see settings.py). If that's
not the case, we'll need to increase the charm revisions
in settings.MINIMUM_REVISIONS_FOR_NEW_API_ENDPOINT
before releasing the new Quickstart.

Tests: `make check`

QA:
- bootstrap quickstart as usual: `devenv/bin/juju-quickstart`;
- check that, if you are using juju devel (1.22beta), quickstart
   properly connect to the new API endpoint;
- run quickstart again to deploy a bundle, e.g.:
   `devenv/bin/juju-quickstart bundle:mediawiki/single`;
- ensure that the deployment request succeeds;
- if possible, do the above with and older version of Juju,
   to ensure backward compatibility.

Done, thank you!

R=martin.hilton, jeff.pihach
CC=
https://codereview.appspot.com/199490043

https://codereview.appspot.com/199490043/

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