Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Fixes unit listing with test update. #470

Merged
merged 1 commit into from Aug 4, 2014

Conversation

jcsackett
Copy link
Contributor

  • Call aggregate update when we add uncommitted units. The unit list and unit
    count update is dependent on this operation firing off.
  • Update tests to ensure this is called.

@jcsackett
Copy link
Contributor Author

To QA

With mv:
Deploy a ghost service; the unit count should be 1, and the 1 uncommitted unit should be in the inspector's unit list.

Add units via scale up (either manual placement or automatic).
The unit count should update, the uncommitted row should update, and when you open the listing you should see all the new units.

Without mv:
Nothing should change with this branch.

@jujugui
Copy link
Contributor

jujugui commented Jul 31, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1505/

@@ -540,7 +540,7 @@ YUI.add('juju-models', function(Y) {
annotations: {},
pending: true,
charm: charm.get('id'),
unit_count: 0, // No units yet.
unit_count: 1, // No units yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to do this here, there shouldn't be any units added to the service at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We create an uncommitted unit immediately, this just updates the unit count for display.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but that's not done here, that's done in ghost-deployer-extension.js via the addUnits call so this is assuming that we make that call. By moving the aggregates update method (mentioned below) into the addUnits call it will update this appropriately every time it's needed.

@jujugui
Copy link
Contributor

jujugui commented Aug 1, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1516/

@jujugui
Copy link
Contributor

jujugui commented Aug 1, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1526/

@jcsackett
Copy link
Contributor Author

Test failure appears to be spurious--everything passes (make check, make test-debug, make test-prod) run locally.

@makyo
Copy link
Contributor

makyo commented Aug 1, 2014

One QA question - The uncommitted stuff works for an uncommitted service, but once the service is deployed, adding units leads to "X undefined units" in the unit list in the inspector. Is this a future branch? If so, 👍

@jcsackett
Copy link
Contributor Author

@makyo Looking into it now; I think that may be a simple oversight, if not let's do it as a follow up.

@kadams54
Copy link
Contributor

kadams54 commented Aug 4, 2014

👍 QA is OK.

@jujugui
Copy link
Contributor

jujugui commented Aug 4, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1535/

@jcsackett
Copy link
Contributor Author

:shipit:

@jujugui
Copy link
Contributor

jujugui commented Aug 4, 2014

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge

@jujugui
Copy link
Contributor

jujugui commented Aug 4, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1537/

jujugui added a commit that referenced this pull request Aug 4, 2014
Fixes unit listing with test update.

* Call aggregate update when we add uncommitted units. The unit list and unit
  count update is dependent on this operation firing off.

* Update tests to ensure this is called.
@jujugui jujugui merged commit 2d08002 into juju:develop Aug 4, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants