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

Remove the mv based dispatch to service inspector vs ghost inspector #576

Merged
merged 3 commits into from Sep 23, 2014

Conversation

jcsackett
Copy link
Contributor

  • Remove the flag conditional in dispatch
  • Remove the ghost service inspector code
  • Remove ghost service insepctor tests

@jcsackett
Copy link
Contributor Author

QA:

Add a service; the service inspector should come up normally.
Make sure the service inspector behaves properly--it should function exactly as it has previously under the MV flag.

@@ -175,7 +175,7 @@
</div>
<script src="/juju-ui/assets/javascripts/spin.min.js"></script>
<script id="app-startup">
//var flags = {}; // Declare an empty set of feature flags.
var flags = {}; // Declare an empty set of feature flags.
startSpinner = function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a drive by fix; this shouldn't have been commented out previously.

@kadams54
Copy link
Contributor

👍 QA is good. Nice work!

QA notes: I added a service under the mv flag, flipped through all the tabs in the pre-commit inspector, and used the scale-up UI to go from 1 to 5 units (I did both auto-deploy and manually). I then repeated these steps without the flag, confirming that they all functioned normally.

topo: topo,
store: topo.get('store')
}, config, true);
inspector = new Y.juju.views.ServiceInspector(inspectorConfig).render();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you look into this a little further. I don't think that this createServiceInspector method is used any longer because we instantiate everything from the browser.js so the whole thing can go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's got a call site in topology.js; there's probably work to be done there excising unused code, but I don't think it's in scope for the main work here, e.g. removing the MV flag.

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

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

@hatched
Copy link
Contributor

hatched commented Sep 23, 2014

Thanks for the investigation into that 👍

@jcsackett
Copy link
Contributor Author

:shipit:

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

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

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/juju-gui-merge/665

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

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

@mitechie
Copy link
Contributor

:shipit:

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

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

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/juju-gui-merge/666

@mitechie
Copy link
Contributor

npm celebrates being all big but we can't get CI through :shipit:

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

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

jujugui added a commit that referenced this pull request Sep 23, 2014
Remove the mv based dispatch to service inspector vs ghost inspector

* Remove the flag conditional in dispatch
* Remove the ghost service inspector code
* Remove ghost service insepctor tests
@jujugui jujugui merged commit e3d6c8f into juju:develop Sep 23, 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