Juan Hernandez has posted comments on this change.
Change subject: webadmin: UI Plugins PoC, revision 5
......................................................................
Patch Set 1: (26 inline comments)
Some mostly minor comments inside.
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 501: select
fn_db_add_config_value('UseSecureConnectionWithServers','true','general');
Line 502: select fn_db_add_config_value('UseVdsBrokerInProc','true','general');
Line 503: select
fn_db_add_config_value('UtilizationThresholdInPercent','80','general');
Line 504: select
fn_db_add_config_value('UIPluginConfigPath','ui-plugins','general');
Line 505: select
fn_db_add_config_value('UIPluginDataPath','ui-plugins','general');
Do we really need to make this configurable? I don't see any reason to add
parameters for this.
Line 506: select fn_db_add_config_value('ValidNumOfMonitors','1,2,4','general');
Line 507: select
fn_db_add_config_value('VcpuConsumptionPercentage','10','general');
Line 508: --Handling Host Installation Bootstrap Script URL
Line 509: select
fn_db_add_config_value('VdcBootStrapUrl','http://example.com/engine/vds_scripts','general');
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/Config.java
Line 101: * DataDir configuration value.
Line 102: *
Line 103: * @return an absolute path for UIPluginDataPath
Line 104: */
Line 105: public static String resolveUIPluginDataPath() {
Instead of using ConfigValues.DataDir I would suggest to use
LocalConfig.getInstance().getUsrDir(). This uses the variable ENGINE_ETC from
/etc/ovirt-engine and has the advantage that can be different in different
engine nodes in an hypothetical cluster.
Line 106: return ConfigUtil.resolvePath(Config.<String>
GetValue(ConfigValues.DataDir),
Line 107: Config.<String>
GetValue(ConfigValues.UIPluginDataPath));
Line 108: }
Line 109:
Line 112: * ConfigDir configuration value.
Line 113: *
Line 114: * @return an absolute path for UIPluginConfigPath
Line 115: */
Line 116: public static String resolveUIPluginConfigPath() {
Same as before, but with getEtcDir().
Line 117: return ConfigUtil.resolvePath(Config.<String>
GetValue(ConfigValues.ConfigDir),
Line 118: Config.<String>
GetValue(ConfigValues.UIPluginConfigPath));
Line 119: }
Line 120:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1529: UIPluginDataPath(391),
Line 1530:
Line 1531: @TypeConverterAttribute(String.class)
Line 1532: @DefaultValueAttribute("ui-plugins")
Line 1533: UIPluginConfigPath(392),
Do we really need these parameters?
Line 1534:
Line 1535: Invalid(65535);
Line 1536:
Line 1537: private int intValue;
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ServletUtils.java
Line 49: public static boolean isSane(String path) {
Line 50: // Check that the path is not too long:
Line 51: final int lenght = path.length();
Line 52: if (lenght > PATH_MAX) {
Line 53: log.error("The path is " + lenght + " characters long,
which is longer than the maximum allowed " + PATH_MAX + ".");
That was my fault, Vojtech just moved it from FileServlet.
Line 54: return false;
Line 55: }
Line 56:
Line 57: // Check that there aren't potentially dangerous directory
navigation sequences:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/plugin/PluginDataManager.java
Line 45: this.pluginConfigDir = new File(pluginConfigPath);
Line 46: this.mapper = createJsonMapper();
Line 47: }
Line 48:
Line 49: ObjectMapper createJsonMapper() {
Shouldn't this be private?
Line 50: ObjectMapper mapper = new ObjectMapper();
Line 51: mapper.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
Line 52: return mapper;
Line 53: }
Line 186: boolean isJsonFile(File pathname) {
Line 187: return pathname.isFile() && pathname.canRead() &&
pathname.getName().endsWith(JSON_FILE_SUFFIX);
Line 188: }
Line 189:
Line 190: JsonNode readJsonNode(File file) {
Private?
Line 191: JsonNode node = null;
Line 192: try {
Line 193: node = mapper.readValue(file, JsonNode.class);
Line 194: } catch (IOException e) {
Line 196: }
Line 197: return node;
Line 198: }
Line 199:
Line 200: JsonNode readConfigurationNode(File configurationFile) {
Private?
Line 201: return isJsonFile(configurationFile) ?
readJsonNode(configurationFile) : null;
Line 202: }
Line 203:
Line 204: String getConfigurationFileName(File descriptorFile) {
Line 200: JsonNode readConfigurationNode(File configurationFile) {
Line 201: return isJsonFile(configurationFile) ?
readJsonNode(configurationFile) : null;
Line 202: }
Line 203:
Line 204: String getConfigurationFileName(File descriptorFile) {
Private?
Line 205: return descriptorFile.getName().replace(JSON_FILE_SUFFIX,
CONFIG_FILE_SUFFIX);
Line 206: }
Line 207:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/plugin/PluginResourceServlet.java
Line 20: * descriptor/configuration data as part of its request handling.
Line 21: */
Line 22: public class PluginResourceServlet extends HttpServlet {
Line 23:
Line 24: private static final long serialVersionUID = 1L;
1? I know this is mostly useless, but can you generate a real id?
Line 25:
Line 26: private static final Logger logger =
Logger.getLogger(PluginResourceServlet.class);
Line 27:
Line 28: private File baseDir;
Line 41: // Ensure non-null request path
Line 42: requestPath = requestPath == null ? slash : requestPath;
Line 43:
Line 44: // Remove leading '/' character
Line 45: requestPath = requestPath.startsWith(slash) ?
requestPath.substring(1) : requestPath;
If you need to remove leading slash it is maybe better to do a loop, in case
there are several.
Line 46:
Line 47: // Split plugin name from relative file path
Line 48: String[] parts = requestPath.split(slash, 2);
Line 49: if (parts.length == 2 && !parts[0].trim().isEmpty() &&
!parts[1].trim().isEmpty()) {
Line 45: requestPath = requestPath.startsWith(slash) ?
requestPath.substring(1) : requestPath;
Line 46:
Line 47: // Split plugin name from relative file path
Line 48: String[] parts = requestPath.split(slash, 2);
Line 49: if (parts.length == 2 && !parts[0].trim().isEmpty() &&
!parts[1].trim().isEmpty()) {
Maybe StringUtils.isNotBlank(parts[0]) && StringUtils.isNotBlank(parts[1]) ?
Line 50: pluginName = parts[0];
Line 51: requestFilePath = parts[1];
Line 52: } else {
Line 53: logger.error("Missing UI plugin name and/or relative file
path for request [" + request.getRequestURI() + "]"); //$NON-NLS-1$
//$NON-NLS-2$
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/plugin/Plugin.java
Line 53: * Verifies if the plugin is currently in one of the given states.
Line 54: * <p>
Line 55: * Returns {@code true} if successful, {@code false} otherwise.
Line 56: */
Line 57: boolean checkCurrentState(List<PluginState> possibleCurrentStates)
{
Private?
Line 58: boolean match = possibleCurrentStates.contains(state);
Line 59: assert match : "Unexpected plugin state [" + state + "],
should be one of: " + possibleCurrentStates; //$NON-NLS-1$ //$NON-NLS-2$
Line 60: return match;
Line 61: }
Line 63: /**
Line 64: * Verifies if the plugin is currently in one of the given states,
and moves plugin state to {@code newState} if
Line 65: * successful.
Line 66: */
Line 67: void moveToState(List<PluginState> possibleCurrentStates,
PluginState newState) {
Private?
Line 68: if (checkCurrentState(possibleCurrentStates)) {
Line 69: state = newState;
Line 70: }
Line 71: }
Line 72:
Line 73: /**
Line 74: * Verifies if the plugin is currently in the given state, and
moves plugin state to {@code newState} if successful.
Line 75: */
Line 76: void moveToState(PluginState possibleCurrentState, PluginState
newState) {
Private?
Line 77: moveToState(Arrays.asList(possibleCurrentState), newState);
Line 78: }
Line 79:
Line 80: public void markAsLoading() {
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/plugin/PluginManager.java
Line 37: exposePluginApi();
Line 38: defineAndLoadPlugins();
Line 39: }
Line 40:
Line 41: Plugin getPlugin(String pluginName) {
Public?
Line 42: return plugins.get(pluginName);
Line 43: }
Line 44:
Line 45: Collection<Plugin> getPlugins() {
Line 41: Plugin getPlugin(String pluginName) {
Line 42: return plugins.get(pluginName);
Line 43: }
Line 44:
Line 45: Collection<Plugin> getPlugins() {
Public?
Line 46: return plugins.values();
Line 47: }
Line 48:
Line 49: void addPlugin(Plugin plugin) {
Line 45: Collection<Plugin> getPlugins() {
Line 46: return plugins.values();
Line 47: }
Line 48:
Line 49: void addPlugin(Plugin plugin) {
Public?
Line 50: plugins.put(plugin.getMetaData().getName(), plugin);
Line 51: }
Line 52:
Line 53: /**
Line 52:
Line 53: /**
Line 54: * Defines all plugins that were detected when serving WebAdmin
host page, and loads them as necessary.
Line 55: */
Line 56: void defineAndLoadPlugins() {
Private?
Line 57: PluginDefinitions definitions = PluginDefinitions.instance();
Line 58:
Line 59: if (definitions != null) {
Line 60: JsArray<PluginMetaData> metaDataArray =
definitions.getMetaDataArray();
Line 71:
Line 72: /**
Line 73: * Defines a plugin from the given meta-data, and loads it as
necessary.
Line 74: */
Line 75: void defineAndLoadPlugin(PluginMetaData pluginMetaData) {
Private?
Line 76: String pluginName = pluginMetaData.getName();
Line 77: String pluginHostPageUrl = pluginMetaData.getHostPageUrl();
Line 78:
Line 79: if (pluginName == null || pluginName.trim().isEmpty()) {
Line 107:
Line 108: /**
Line 109: * Loads the given plugin by attaching the corresponding iframe
element to DOM.
Line 110: */
Line 111: void loadPlugin(Plugin plugin) {
Private?
Line 112: if (plugin.isInState(PluginState.DEFINED)) {
Line 113: logger.info("Loading plugin [" +
plugin.getMetaData().getName() + "]"); //$NON-NLS-1$ //$NON-NLS-2$
Line 114:
Document.get().getBody().appendChild(plugin.getIFrameElement());
Line 115: plugin.markAsLoading();
Line 204: * <li>the plugin is either {@linkplain PluginState#INITIALIZING
initializing} (actions performed from UiInit
Line 205: * function), or {@linkplain PluginState#IN_USE in use} (actions
performed from other event handler functions)
Line 206: * </ul>
Line 207: */
Line 208: boolean canDoPluginAction(String pluginName) {
Private?
Line 209: Plugin plugin = getPlugin(pluginName);
Line 210: boolean pluginInitializingOrInUse = plugin != null
Line 211: ? plugin.isInState(PluginState.INITIALIZING) ||
plugin.isInState(PluginState.IN_USE) : false;
Line 212: return canInvokePlugins && pluginInitializingOrInUse;
Line 214:
Line 215: /**
Line 216: * Registers an event handler object (object containing plugin
event handler functions) for the given plugin.
Line 217: */
Line 218: void registerPluginEventHandlerObject(String pluginName,
JavaScriptObject pluginEventHandlerObject) {
Private?
Line 219: Plugin plugin = getPlugin(pluginName);
Line 220:
Line 221: if (plugin != null && pluginEventHandlerObject != null) {
Line 222: plugin.setEventHandlerObject(pluginEventHandlerObject);
Line 260: * <p>
Line 261: * If UiInit function fails due to uncaught exception, the plugin
will be {@linkplain PluginState#FAILED removed
Line 262: * from service}.
Line 263: */
Line 264: void initPlugin(String pluginName) {
Private?
Line 265: Plugin plugin = getPlugin(pluginName);
Line 266:
Line 267: if (canInvokePlugins && plugin != null &&
plugin.isInState(PluginState.READY)) {
Line 268: logger.info("Initializing plugin [" + pluginName + "]");
//$NON-NLS-1$ //$NON-NLS-2$
Line 280:
Line 281: /**
Line 282: * Removes the given plugin from service due to plugin failure.
Line 283: */
Line 284: void pluginFailed(Plugin plugin) {
Private?
Line 285:
Document.get().getBody().removeChild(plugin.getIFrameElement());
Line 286: plugin.markAsFailed();
Line 287: logger.warning("Plugin [" + plugin.getMetaData().getName() +
"] removed from service due to failure"); //$NON-NLS-1$ //$NON-NLS-2$
Line 288: }
Line 289:
Line 290: /**
Line 291: * Returns the configuration object associated with the given
plugin, or {@code null} if no such object exists.
Line 292: */
Line 293: JavaScriptObject getConfigObject(String pluginName) {
Public?
Line 294: Plugin plugin = getPlugin(pluginName);
Line 295: return plugin != null ?
plugin.getMetaData().getConfigObject() : null;
Line 296: }
Line 297:
--
To view, visit http://gerrit.ovirt.org/8120
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibbf7659f023d8929fb0a8303061851bb7ee555bc
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
