Ginger should NOT depeden on Ginger s390x #317

alinefm · 2016-04-14T23:41:12Z

While testing Ginger, I noticed the "Storage Devices" section has an Add button. When selecting it I got the following view:

As you can suppose, I am not running Ginger s390x and because that the 404 Not Found error.

After that, looking at the code, I notice Ginger UI is doing requests to Ginger s390x API.
AFAIK, Ginger should not depend on Ginger s390x, right?

IMO it needs to be fixed ASAP.

ui/js/host-network.js:        if ($.inArray("gingers390x", result) != -1) {
ui/js/host-network.js:          wok.window.open('plugins/gingers390x/network.html');
ui/js/host-network.js:  // Remove button "Add Adapter" in case of architecture != s390x
ui/js/host-network.js:  if (ginger.hostarch != "s390x") {
ui/js/host-network.js:    if (ginger.hostarch != "s390x") {
ui/js/host-network.js:  // check architecture and gingers390x plugin for ethernet deletion
ui/js/host-network.js:  if (ginger.hostarch == 's390x') {
ui/js/host-network.js:      if ($.inArray("gingers390x", result) != -1) {
ui/js/host-network.js:        // display message asking user to install gingers390x plugin to avail delete Ethernet interfaces functionality
ui/js/host-network.js:      // failed check gingers390x plugin
ui/js/host-network.js:    // if not s390x architecture, display error message that deletion of ethernet
ui/js/host-storage.js:      if ((ginger.hostarch == "s390x") && ($.inArray("gingers390x", result) != -1)) {
ui/js/host-storage.js:  if (ginger.hostarch == 's390x') {
ui/js/host-storage.js:    if ((ginger.hostarch == "s390x") && ($.inArray("gingers390x", result) != -1)) {
ui/js/host-storage.js:      wok.window.open('plugins/gingers390x/fcpsanadapter.html');
ui/js/host-storage.js:        $('#sd-add-FCP-button').attr('href', 'plugins/gingers390x/addFCPLuns.html');
ui/js/host-storage.js:        wok.window.open('plugins/gingers390x/eckd.html');
ui/js/host-storage.js:      if ((ginger.hostarch == "s390x") && ($.inArray("gingers390x", result) != -1)) {
ui/js/util.js:s390xtrackingTasks = [];
ui/js/util.js:      ginger.tracks390xTask(taskID, suc, err, progress);
ui/js/util.js:      url : 'plugins/gingers390x/nwdevices/' + name + '/unconfigure',
ui/js/util.js:        url : 'plugins/gingers390x/lstapes',
ui/js/util.js:        url : "/plugins/gingers390x/storagedevices/"+ busId +"/offline",
ui/js/util.js:        url : "/plugins/gingers390x/fcluns/"+ lunPath,
ui/js/util.js:ginger.gets390xTask = function(taskId, suc, err) {
ui/js/util.js:      url: 'plugins/gingers390x/tasks/' + encodeURIComponent(taskId),
ui/js/util.js:ginger.tracks390xTask = function(taskID, suc, err, progress) {
ui/js/util.js:                ginger.tracks390xTask(taskID, suc, err, progress);
ui/js/util.js:    ginger.gets390xTask(taskID, onTaskResponse, err);
ui/js/util.js:    if(s390xtrackingTasks.indexOf(taskID) < 0)
ui/js/util.js:        s390xtrackingTasks.push(taskID);
ui/js/util.js:    url: 'plugins/gingers390x/lunscan',
ui/pages/help/en_US/host-storage.dita:<dd>FCP Tape device is any computing hardware that is used for storing, porting and extracting data files and objects. This feature is available for s390x architecture.</dd>
ui/pages/help/en_US/host-storage.dita:<dd>A SAN Adapter is IO device used to connect to the remote SAN storage devices. This feature is available for s390x architecture.</dd>
ui/pages/i18n.json.tmpl:    "GINNET0014M": "$_("Plugin Ginger s390x not installed. Please install plugin 'Ginger s390x' to avail 'Add Adapter' functionality")",
ui/pages/i18n.json.tmpl:    "GINNET0029M": "$_("Please install plugin 'Ginger s390x' to avail 'Delete' functionality for Ethernet interface.")",
ui/pages/i18n.json.tmpl:    "GINNET0031M": "$_("Failed to verify 'Ginger s390x' plugin installation. Please try again.")",

The last errors messages scares me about making Ginger s390x a Ginger dependency,

The text was updated successfully, but these errors were encountered:

danielhb · 2016-04-15T00:29:37Z

Gingers390x isn't a Ginger dependency, it's the other way around.

What you're seeing here might have a simpler explanation: there is no 'Add adapter' backend implemented in Ginger for Intel or Power. This is why you're seeing the UI doing this verification.

This is obviously a lousy design and we need to take action. I see 2 alternatives:

1 - move the said API to Ginger, keep the 's390x' hostarch restriction in the UI

or

2 - remove this UI code from the Ginger UI and put it into s390x UI. s390x should have its own ways to extend this tab and add its own functionalities.

I like (1) better because the 'Add adapter' option in the Storage Devices tab will be implemented sooner or later for other architectures and we're better of if the API is hosted in Ginger.

@chandrureddy let me know what you think about this issue and the proposed solutions.

alinefm · 2016-04-15T00:36:54Z

@danielhb The API provided by Ginger s390x is only to add FCP and ECKD devices which are exclusively for Z systems (from my quick search over the internet) so there is no reason to move those APIs to Ginger.

Said that, Ginger s390x should be able to extend Ginger UI without making Ginger dependent of it. So go for your option 2.

danielhb · 2016-04-15T01:09:32Z

Ok. Option 2 it is

We'll need a new bug @ s390x to add this button, extending this Ginger UI,
while we remove this UI code from Ginger. I would prefer to apply both
changes at the same time.
Em 14/04/2016 9:36 PM, "Aline Manera" notifications@github.com escreveu:

@danielhb https://github.com/danielhb The API provided by Ginger s390x
is only to add FCP and ECKD devices which are exclusively for Z systems
(from my quick search over the internet) so there is no reason to move
those APIs to Ginger.

Said that, Ginger s390x should be able to extend Ginger UI without making
Ginger dependent of it. So go for your option 2.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#317 (comment)

potula-chandra · 2016-04-15T06:37:02Z

On 4/15/16 5:59 AM, Daniel Henrique Barboza wrote:

Gingers390x isn't a Ginger dependency, it's the other way around.

What you're seeing here might have a simpler explanation: there is no
'Add adapter' backend implemented in Ginger for Intel or Power. This
is why you're seeing the UI doing this verification.

This is obviously a lousy design and we need to take action. I see 2
alternatives:

1 - move the said API to Ginger, keep the 's390x' hostarch restriction
in the UI

The reason we carved out s390x plugin is to keep s390x specific
functionality separate and have clean isolation for the functionality
which is specific to a platform (in this case it is s390x). It is not
good either for Ginger to deliver all the code to all the platforms
where it doesn't make sense at all.
So this option is not good in my opinion.

or

2 - remove this UI code from the Ginger UI and put it into s390x UI.
s390x should have its own ways to extend this tab and add its own
functionalities.

Let me explain, Ginger is enabler for the s390x UI/backend code and
Ginger do not implement the actual functionality. Ginger is just helping
by showing additional options in the UI in case of s390x(if only s390x
plugin installed) environment. I do not see any issue here.

Also Ginger in no means have dependency to Ginger s390x. It is just that
show additional features it if ran on s390x architecture(if only s390x
plugin installed) . There is not tight dependency here between Ginger
and Ginger S390x. I agree few checks we are making but not a big trouble.

In my opinion this option looks ugly if we have to come up with
additional tabs for the specific platform functionality. This will be
annoying to the user if he/she has to switch many tabs to achieve one
use case fully.

I like (1) better because the 'Add adapter' option in the Storage
Devices tab will be implemented sooner or later for other
architectures and we're better of if the API is hosted in Ginger.

@chandrureddy https://github.com/chandrureddy let me know what you
think about this issue and the proposed solutions.

To conclude I do not find both options good and I neither see big
trouble with the existing implementation. From the user experience point
of view both options will not serve the purpose better.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#317 (comment)

potula-chandra · 2016-04-15T11:05:37Z

But I guess above issue can be fixed as early as possible !!!

danielhb · 2016-04-15T12:43:08Z

Let's talk about the user experience point of view. I have a s390x machine and want to try Ginger. Ginger depends on WoK and Gingerbase, thus I install Wok+Gingerbase+Ginger. "Add Adapter" button will show because the check is being made for arch = 's390x' instead of checking for gingers390x. The 'onClick' will not work because is checking for gingers390x.

So I'll have a button that does nothing because I haven't installed gingers390x and no way of knowing why it's not working. We can't take for granted that anyone running Ginger in s390x will have the Gingers390x plug-in. The UI isn't helping either, it's not informing the user about the missing plug-in, thus the user will not even be aware that it's missing. This is a true user experience problem but can be amended by adding gingers390x checks all over the place.

The other problem, however, is more complex and it concerns more about Gingers390x than Ginger. Ginger devs can look at this UI code, think 'wow I do not like this', turn a blind eye and that's it. What about the gingers390x devs? This dev will have an API that's implemented in Gingers390x but the UI is in Ginger. Any backend change in this API in gingers390x will require a UI update in Ginger. I know that today we're talking about the same community, even the same maintainer. However it can be different in the future. Perhaps gingers390x will grow and get its own community, maintainer and agenda apart from Ginger. When you think in these terms having an API in one plug-in and the UI in the other will compromise Gingers390x development directly.

I understand that both (1) and (2) options have drawbacks but, for the sake of the Gingers390x coding experience, we can't have the backend there and the UI in Ginger. I will not say that this is a urgent, must fix now bug, but definitely this is something we'll need fixing rather sooner than later.

laggarcia · 2016-04-15T13:34:45Z

On 04/15/2016 09:43 AM, Daniel Henrique Barboza wrote:

Let's talk about the user experience point of view. I have a s390x
machine and want to try Ginger. Ginger depends on WoK and Gingerbase,
thus I install Wok+Gingerbase+Ginger. "Add Adapter" button will show
because the check is being made for arch = 's390x' instead of checking
for gingers390x. The 'onClick' will not work because is checking for
gingers390x.

So I'll have a button that does nothing because I haven't installed
gingers390x and no way of knowing why it's not working. We can't take
for granted that anyone running Ginger in s390x will have the
Gingers390x plug-in. The UI isn't helping either, it's not informing
the user about the missing plug-in, thus the user will not even be
aware that it's missing. This is a true user experience problem but
can be amended by adding gingers390x all over the place.

The other problem, however, is more complex and it concerns more about
Gingers390x than Ginger. Ginger devs can look at this UI code, think
'wow I do not like this', turn a blind eye and that's it. What about
the gingers390x devs? This dev will have an API that's implemented in
Gingers390x but the UI is in Ginger. Any backend change in this API in
gingers390x will require a UI update in Ginger. I know that today
we're talking about the same community, even the same maintainer.
However it can be different in the future. Perhaps gingers390x will
grow and get its own community, maintainer and agenda apart from
Ginger. When you think in these terms having an API in one plug-in and
the UI in the other will compromise Gingers390x development directly.

I agree with all that. And, TBH, I think the best exit would be to
eliminate Ginger s390x plug-in at all and merge all its content in
Ginger. Ginger already has a very neat feature that checks if a feature
is supported in a platform or not and enable/disable it accordingly.
Unless I am missing something, I think Ginger s390x should just be part
of Ginger with the necessary checks for s390x architecture. I can't see
a technical reason for this separation anymore, and it is the same
community in the end, as @danielhb said.

I understand that both (1) and (2) options have drawbacks but, for the
sake of the Gingers390x coding experience, we can't have the backend
there and the UI in Ginger. I will not say that this is a urgent, must
fix now bug, but definitely this is something we'll need fixing rather
sooner than later.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#317 (comment)

laggarcia · 2016-04-15T13:49:11Z

On 04/15/2016 03:37 AM, Chandra Shekhar Reddy wrote:

On 4/15/16 5:59 AM, Daniel Henrique Barboza wrote:

Gingers390x isn't a Ginger dependency, it's the other way around.

What you're seeing here might have a simpler explanation: there is no
'Add adapter' backend implemented in Ginger for Intel or Power. This
is why you're seeing the UI doing this verification.

This is obviously a lousy design and we need to take action. I see 2
alternatives:

1 - move the said API to Ginger, keep the 's390x' hostarch restriction
in the UI

The reason we carved out s390x plugin is to keep s390x specific
functionality separate and have clean isolation for the functionality
which is specific to a platform (in this case it is s390x). It is not
good either for Ginger to deliver all the code to all the platforms
where it doesn't make sense at all.
So this option is not good in my opinion.

or

2 - remove this UI code from the Ginger UI and put it into s390x UI.
s390x should have its own ways to extend this tab and add its own
functionalities.

Let me explain, Ginger is enabler for the s390x UI/backend code and
Ginger do not implement the actual functionality. Ginger is just helping
by showing additional options in the UI in case of s390x(if only s390x
plugin installed) environment. I do not see any issue here.

Well, that doesn't seem to be true according to what @alinefm wrote
here. She doesn't have Ginger s390x installed and the UI is not working
correctly on other platforms.

Also Ginger in no means have dependency to Ginger s390x. It is just that
show additional features it if ran on s390x architecture(if only s390x
plugin installed) . There is not tight dependency here between Ginger
and Ginger S390x. I agree few checks we are making but not a big trouble.

If you are explicitly checking for Ginger s390x inside Ginger code, that
means Ginger has a dependency on Ginger s390x and this is no good. We
shall remove this kind of check from Ginger. Ginger s390x should be
self-contained.

In my opinion this option looks ugly if we have to come up with
additional tabs for the specific platform functionality. This will be
annoying to the user if he/she has to switch many tabs to achieve one
use case fully.

I like (1) better because the 'Add adapter' option in the Storage
Devices tab will be implemented sooner or later for other
architectures and we're better of if the API is hosted in Ginger.

@chandrureddy https://github.com/chandrureddy let me know what you
think about this issue and the proposed solutions.

To conclude I do not find both options good and I neither see big
trouble with the existing implementation. From the user experience point
of view both options will not serve the purpose better.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

#317 (comment)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#317 (comment)

alinefm · 2016-04-15T17:25:53Z

@chandrureddy I don't agree in having Ginger s390x references in Ginger source code.
As Leonardo said, the plugin should be self-contained.

I have already sent a detailed idea to solve that to the Ginger ML, but I will also post here so everyone can be aware and comment.

We need to remove any reference to s390x from Ginger source code.
The plugins must have independence. Continue doing it that way, means any changes on s390x may impact on Ginger source code which is really bad.

My suggestion is to create a mechanism to inform Ginger needs to load an additional JS file from gingers390x. That gingers390x JS will do what the code in Ginger does today.
That will involves changes on Wok too. So we can make any plugin UI extended by another.

Said that, let's talk about the technical details:

Move any Ginger s390x JS code to Ginger s390x source code.
Add a new parameter to WokRoot() in src/wok/root.py

    class WokRoot(Root):
        ...
        self.extends = None

Proper add self.extends to Gingers390x() in src/wok/plugins/gingers390x/gingers390x.py

    class Gingers390x(WokRoot):
        ...

        self.extends = {
            "/plugins/ginger": {
                "host-network.html": "<path to s390x network JS>",
                "host-storage.html": "<path to s390x storage JS>"
            }
        }

Change Root.tabs() in src/wok/root.py as below:

    class Root():
        ...
        def tabs(...):
            ...
            # This will contain the additional scripts to be loaded in the tab HTML.
            data['scripts'] = []

            # Check all plugins to get the extends values and parse it as needed.
            for plugin, app in cherrypy.tree.apps.iteritems():
                if app.root.extends is not None:
                    scripts = app.root.extends.get(script_name, {})
                    if page in scripts.keys():
                        data['scripts'].append(scripts[page])
            ...

Fix render_cheetah_file() in src/wok/template.py to do not override 'data' parameter.

    ...
    params['data'] = data
    ...

Change the tabs HTML files to support 'scripts' data.
In this specific case, Ginger and Ginger s390x issue, you will need to change ui/pages/tabs/host-storage.html.tmpl and ui/pages/tabs/host-network.html.tmpl as following:

<head>                                                                         
...
#for $script in $data.scripts
<script type="text/javascript" src="$script"></script>
#end for
</head>

And THAT IS ALL! \o/
That way, Ginger s390x will extend Ginger UI without adding any reference on Ginger.

I think that is the best solution to the problem right now.
I can help with the patches for it if needed.

Let me know your thoughts on it.

danielhb · 2016-04-15T17:55:53Z

I approve Aline's idea. It will be the best approach for the UI setup we
have today.

On 04/15/2016 02:25 PM, Aline Manera wrote:

@chandrureddy https://github.com/chandrureddy I don't agree in
having Ginger s390x references in Ginger source code.
As Leonardo said, the plugin should be self-contained.

I have already sent a detailed idea to solve that to the Ginger ML,
but I will also post here so everyone can be aware and comment.

We need to remove any reference to s390x from Ginger source code.
The plugins must have independence. Continue doing it that way, means
any changes on s390x may impact on Ginger source code which is really bad.

My suggestion is to create a mechanism to inform Ginger needs to load
an additional JS file from gingers390x. That gingers390x JS will do
what the code in Ginger does today.
That will involves changes on Wok too. So we can make any plugin UI
extended by another.

Said that, let's talk about the technical details:

Move any Ginger s390x JS code to Ginger s390x source code.

Add a new parameter to WokRoot() in src/wok/root.py

|class WokRoot(Root): ... self.extends = None |

Proper add self.extends to Gingers390x() in
src/wok/plugins/gingers390x/gingers390x.py

|class Gingers390x(WokRoot): ... self.extends = { "ginger": {
"host-network.html": "",
"host-storage.html": "" } } |

Change Root.tabs() in src/wok/root.py as below:

|class Root(): ... def tabs(...): ... # This will contain the
additional scripts to be loaded in the tab HTML. data['scripts'] = []

Check all plugins to get the extends values and parse it as needed.

for plugin, app in cherrypy.tree.apps.iteritems(): if app.root.extends
is not None: scripts = app.root.extends.get('script_name', {}) if page
in scripts.keys(): data['scripts'].append(scripts[page]) ... |

Fix render_cheetah_file() in src/wok/template.py to do not
override 'data' parameter.

|... params['data'] = data ... |

Change the tabs HTML files to support 'scripts' data. In this
specific case, Ginger and Ginger s390x issue, you will need to
change ui/pages/tabs/host-storage.html.tmpl and
ui/pages/tabs/host-network.html.tmpl as following:

| <script type="text/javascript" src="plugins/ginger/js/util.js"></script> <script type="text/javascript" src="plugins/ginger/js/host-storage.js"></script> <script type="text/javascript" src="plugins/ginger/js/ginger-bootgrid.js"></script> #for $script in
$data.scripts <script type="text/javascript" src="$script"></script>
#end for |

And THAS IS ALL! \o/
That way, Ginger s390x will extend Ginger UI without adding any
reference on Ginger.

I think that is the best solution to the problem right now.
I can help with the patches for it if needed.
Let me know your thoughts on it.
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#317 (comment)

potula-chandra · 2016-04-16T06:07:34Z

@alinefm : Thanks for the suggestion
I agree and let us give it a try.

atreyeemukhopadhyay · 2016-04-20T09:12:10Z

@alinefm , i followed the steps you mentioned and I am able to fix the issue. Thank you !!.
I am cleaning gingers390x reference from ginger and will be sending patch as soon as possible.

Removing gingers390x related changes from ginger js files.

Importing dependent gingers390x js files in Ginger tab files.

Removing gingers390x specific messages from ginger i18n file.

GingerS390x references are deleted from storag help page.

Signed-off-by: Daniel Henrique Barboza <dhbarboza82@gmail.com>

alinefm added bug UI labels Apr 14, 2016

potula-chandra mentioned this issue Apr 18, 2016

Redesign "Storage Devices" UI #307

Closed

potula-chandra assigned atreyeemukhopadhyay Apr 20, 2016

alinefm added this to the Ginger 2.2 milestone Apr 28, 2016

danielhb pushed a commit that referenced this issue May 3, 2016

Issue #317: Ginger should NOT depend on Gingers390x.

d27149c

Removing gingers390x related changes from ginger js files.

danielhb pushed a commit that referenced this issue May 3, 2016

Issue #317: Ginger should NOT depend on Gingers390x

53cc370

Importing dependent gingers390x js files in Ginger tab files.

danielhb pushed a commit that referenced this issue May 3, 2016

Issue #317: Ginger should NOT depend on Gingers390x

18420a0

Removing gingers390x specific messages from ginger i18n file.

danielhb pushed a commit that referenced this issue May 3, 2016

Issue #317: Ginger should NOT depend on Gingers390x

54efb97

GingerS390x references are deleted from storag help page.

danielhb added a commit that referenced this issue May 3, 2016

Issue #317: removing 's390x' references from po files

9f6010f

Signed-off-by: Daniel Henrique Barboza <dhbarboza82@gmail.com>

alinefm closed this as completed May 3, 2016

potula-chandra mentioned this issue May 4, 2016

No option to Add Bond/ Add vlan from UI. #314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ginger should NOT depeden on Ginger s390x #317

Ginger should NOT depeden on Ginger s390x #317

alinefm commented Apr 14, 2016 •

edited

danielhb commented Apr 15, 2016

alinefm commented Apr 15, 2016

danielhb commented Apr 15, 2016

potula-chandra commented Apr 15, 2016

potula-chandra commented Apr 15, 2016

danielhb commented Apr 15, 2016 •

edited

laggarcia commented Apr 15, 2016

laggarcia commented Apr 15, 2016

alinefm commented Apr 15, 2016 •

edited

danielhb commented Apr 15, 2016

Check all plugins to get the extends values and parse it as needed.

potula-chandra commented Apr 16, 2016

atreyeemukhopadhyay commented Apr 20, 2016

Ginger should NOT depeden on Ginger s390x #317

Ginger should NOT depeden on Ginger s390x #317

Comments

alinefm commented Apr 14, 2016 • edited

danielhb commented Apr 15, 2016

alinefm commented Apr 15, 2016

danielhb commented Apr 15, 2016

potula-chandra commented Apr 15, 2016

potula-chandra commented Apr 15, 2016

danielhb commented Apr 15, 2016 • edited

laggarcia commented Apr 15, 2016

laggarcia commented Apr 15, 2016

alinefm commented Apr 15, 2016 • edited

danielhb commented Apr 15, 2016

Check all plugins to get the extends values and parse it as needed.

potula-chandra commented Apr 16, 2016

atreyeemukhopadhyay commented Apr 20, 2016

alinefm commented Apr 14, 2016 •

edited

danielhb commented Apr 15, 2016 •

edited

alinefm commented Apr 15, 2016 •

edited