Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parameter descriptions for operations. HWKAGENT-87. #229

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,10 @@
<operation-dmr name="JDR" operation-name="generate-jdr-report" path="/subsystem=jdr" />
<operation-dmr name="Reload" operation-name="reload" />
<operation-dmr name="Resume" operation-name="resume" />
<operation-dmr name="Shutdown" operation-name="shutdown" /> <!-- shutdown and restart -->
<operation-dmr name="Shutdown" operation-name="shutdown" > <!-- shutdown and restart -->
Copy link
Contributor

Choose a reason for hiding this comment

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

@pilhuhn should we take the opportunity now and rename "operation-name" to something less confusing like "internal-name" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

On 12 Jul 2016, at 13:45, John Mazzitelli wrote:

@@ -609,7 +609,10 @@


  •    <operation-dmr name="Shutdown" operation-name="shutdown" /> 
    
  •    <operation-dmr name="Shutdown" operation-name="shutdown" > 
    

@pilhuhn should we take the opportunity now and rename
"operation-name" to something less confusing like "internal-name" ?

I would appreciate this. Bonus: it is transparent for users/clients.

<param name="timeout" type="number" defaultValue="0" description="Timeout in seconds to allow active connections to drain"/>
<param name="restart" type="bool" defaultValue="false" description="Should the server be restarted after shutdown?"/>
</operation-dmr>
<operation-dmr name="Suspend" operation-name="suspend" />
<operation-dmr name="Deploy" operation-name="deploy" />
<operation-dmr name="Undeploy" operation-name="undeploy" />
Expand All @@ -627,11 +630,15 @@
<resource-config-dmr name="Local Host Name" attribute="local-host-name" />

<operation-dmr name="Reload Servers" operation-name="reload-servers" />
<operation-dmr name="Restart Servers" operation-name="restart-servers" />
<operation-dmr name="Restart Servers" operation-name="restart-servers" >
<param name="timeout" type="number" defaultValue="0" description="Timeout in seconds to allow active connections to drain"/>
</operation-dmr>
<operation-dmr name="Resume Servers" operation-name="resume-servers" />
<operation-dmr name="Start Servers" operation-name="start-servers" />
<operation-dmr name="Stop Servers" operation-name="stop-servers" />
<operation-dmr name="Suspend Servers" operation-name="suspend-servers" />
<operation-dmr name="Suspend Servers" operation-name="suspend-servers" >
<param name="timeout" type="number" defaultValue="0" description="Timeout in seconds to allow active connections to drain"/>
</operation-dmr>
<operation-dmr name="Deploy" operation-name="deploy" />
<operation-dmr name="Undeploy" operation-name="undeploy" />
</resource-type-dmr>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ public interface DMROperationAttributes {
.addFlag(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES)
.build();


AttributeDefinition[] ATTRIBUTES = {
PATH,
OPERATION_NAME
OPERATION_NAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comma - btw: this OPERATION_NAME is the thing we should rename to something like INTERNAL_NAME

};
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015 Red Hat, Inc. and/or its affiliates
* Copyright 2015-2016 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -18,6 +18,7 @@

import java.util.Arrays;
import java.util.Collection;
import java.util.List;

import org.jboss.as.controller.AttributeDefinition;
import org.jboss.as.controller.PathElement;
Expand All @@ -44,4 +45,9 @@ private DMROperationDefinition() {
public Collection<AttributeDefinition> getAttributes() {
return Arrays.asList(DMROperationAttributes.ATTRIBUTES);
}

@Override
protected List<? extends PersistentResourceDefinition> getChildren() {
return Arrays.asList(DMROperationParamDefinition.INSTANCE);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright 2015-2016 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.hawkular.agent.monitor.extension;

import org.jboss.as.controller.AbstractAddStepHandler;

public class DMROperationParamAdd extends AbstractAddStepHandler {

public static final DMROperationParamAdd INSTANCE = new DMROperationParamAdd();

private DMROperationParamAdd() {
super(DMROperationParamAttributes.ATTRIBUTES);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright 2015-2016 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.hawkular.agent.monitor.extension;

import org.jboss.as.controller.SimpleAttributeDefinition;
import org.jboss.as.controller.SimpleAttributeDefinitionBuilder;
import org.jboss.as.controller.registry.AttributeAccess;
import org.jboss.dmr.ModelType;

public interface DMROperationParamAttributes {


SimpleAttributeDefinition TYPE = new SimpleAttributeDefinitionBuilder("type",
ModelType.STRING)
.setAllowNull(false)
Copy link
Contributor

@jmazzitelli jmazzitelli Jul 12, 2016

Choose a reason for hiding this comment

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

in the .xsd, you have:
<xs:attribute name="type" type="paramTypeType" use="optional" default="string"/>

if the default is "string" we need to specify that here via:

.setDefaultValue(new ModelNode("string"))

.setAllowExpression(true)
.addFlag(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES)
.build();

SimpleAttributeDefinition DEFAULT_VALUE = new SimpleAttributeDefinitionBuilder("defaultValue",
Copy link
Contributor

Choose a reason for hiding this comment

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

all attribute names need to follow wildfly standards - no camel-case. this should be "default-value" not "defaultValue"

ModelType.STRING)
.setAllowNull(true)
.setAllowExpression(true)
.addFlag(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES)
.build();

SimpleAttributeDefinition DESCRIPTION = new SimpleAttributeDefinitionBuilder("description",
ModelType.STRING)
.setAllowNull(true)
.setAllowExpression(true)
.addFlag(AttributeAccess.Flag.RESTART_RESOURCE_SERVICES)
.build();

SimpleAttributeDefinition[] ATTRIBUTES = {
TYPE,
DEFAULT_VALUE,
DESCRIPTION
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright 2015-2016 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.hawkular.agent.monitor.extension;

import java.util.Arrays;
import java.util.Collection;

import org.jboss.as.controller.AttributeDefinition;
import org.jboss.as.controller.PathElement;
import org.jboss.as.controller.PersistentResourceDefinition;
import org.jboss.as.controller.registry.OperationEntry.Flag;

public class DMROperationParamDefinition extends PersistentResourceDefinition {

public static final DMROperationParamDefinition INSTANCE = new DMROperationParamDefinition();

static final String PARAM = "param";

private DMROperationParamDefinition() {
super(PathElement.pathElement(PARAM),
SubsystemExtension.getResourceDescriptionResolver(DMRResourceTypeSetDefinition.RESOURCE_TYPE_SET,
DMRResourceTypeDefinition.RESOURCE_TYPE, DMROperationDefinition.OPERATION, PARAM),
DMROperationParamAdd.INSTANCE,
DMROperationParamRemove.INSTANCE,
Flag.RESTART_RESOURCE_SERVICES,
Flag.RESTART_RESOURCE_SERVICES);
}


@Override
public Collection<AttributeDefinition> getAttributes() {
return Arrays.asList(DMROperationParamAttributes.ATTRIBUTES);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright 2015-2016 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.hawkular.agent.monitor.extension;

import org.jboss.as.controller.AbstractRemoveStepHandler;

public class DMROperationParamRemove extends AbstractRemoveStepHandler {

public static final DMROperationParamRemove INSTANCE = new DMROperationParamRemove();

private DMROperationParamRemove() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.hawkular.agent.monitor.inventory.NameSet;
import org.hawkular.agent.monitor.inventory.NameSet.NameSetBuilder;
import org.hawkular.agent.monitor.inventory.Operation;
import org.hawkular.agent.monitor.inventory.OperationParam;
import org.hawkular.agent.monitor.inventory.ResourceConfigurationPropertyType;
import org.hawkular.agent.monitor.inventory.ResourceType;
import org.hawkular.agent.monitor.inventory.ResourceType.Builder;
Expand All @@ -70,6 +71,7 @@
import org.jboss.as.controller.SimpleAttributeDefinition;
import org.jboss.as.controller.client.helpers.MeasurementUnit;
import org.jboss.dmr.ModelNode;
import org.jboss.dmr.ModelType;
import org.jboss.dmr.Property;

/**
Expand Down Expand Up @@ -439,7 +441,7 @@ private static void determineMetricSetPrometheus(ModelNode config,

}

private static String determinePlatformMachineId(ModelNode config, OperationContext context)
private static String determinePlatformMachineId(ModelNode config, OperationContext context)
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial - reformat - there's two extra spaces here

throws OperationFailedException {

if (!config.hasDefined(PlatformDefinition.PLATFORM)) {
Expand Down Expand Up @@ -971,13 +973,16 @@ private static void determineResourceTypeSetDmr(ModelNode config,
List<Property> operationList = opModelNode.asPropertyList();
for (Property operationProperty : operationList) {
ModelNode operationValueNode = operationProperty.getValue();
String operationName = operationProperty.getName();
String name = operationProperty.getName();

List<OperationParam> params = getOpParamListFromOpNode(operationValueNode);

PathAddress pathAddress = getPath(operationValueNode, context,
DMROperationAttributes.PATH);
Operation<DMRNodeLocation> op = new Operation<>(ID.NULL_ID, new Name(operationName),
String operationName = getString(operationValueNode, context, DMROperationAttributes.OPERATION_NAME);
Operation<DMRNodeLocation> op = new Operation<>(ID.NULL_ID, new Name(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

this changes the operation ID from the internal name to the display name. I think that is incorrect (?) When NULL is passed in the ID param, the Name also becomes the ID. To keep the ID the same as it was before, you'd need to change the first argument from ID.NULL_ID to "new ID(operationName)"

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed operationName to name some lines above as it was more logical to me, but seems to confuse you now :)

new DMRNodeLocation(pathAddress),
getString(operationValueNode, context, DMROperationAttributes.OPERATION_NAME));
operationName, params);
resourceTypeBuilder.operation(op);
}
}
Expand Down Expand Up @@ -1090,7 +1095,7 @@ private static void determineResourceTypeSetJmx(ModelNode config,
new JMXNodeLocation(getObjectName(operationValueNode, context,
JMXOperationAttributes.OBJECT_NAME)),
getString(operationValueNode, context,
JMXOperationAttributes.OPERATION_NAME));
JMXOperationAttributes.OPERATION_NAME),null);
resourceTypeBuilder.operation(op);
}
}
Expand Down Expand Up @@ -1377,6 +1382,33 @@ private static List<Name> getNameListFromString(ModelNode modelNode, OperationCo
}
}

private static List<OperationParam> getOpParamListFromOpNode(ModelNode modelNode) throws OperationFailedException {

List<OperationParam> ret = new ArrayList<>();

ModelNode params = modelNode.get("param");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is hardcoding "param" - need to use the Attribute class and resolve expressions. See "getString" usages as examples

if (params.getType()==ModelType.UNDEFINED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial - java format used in the project dictates spaces around "=="

return Collections.emptyList();
}
List<Property> ps = params.asPropertyList();
for (Property prop : ps) {
String name = prop.getName();
ModelNode m = prop.getValue();
String typ = m.get("type").asString();
Copy link
Contributor

Choose a reason for hiding this comment

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

do not hardcode "type" - use the constant in the Attribute class. need to use the Attribute class and resolve expressions. See "getString" usages as examples

String desc = m.get("description").asString();
Copy link
Contributor

Choose a reason for hiding this comment

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

do not hardcode "description" - use the constant in the Attribute class. need to use the Attribute class and resolve expressions. See "getString" usages as examples


OperationParam operationParam = new OperationParam(name,typ,desc);
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial : java format dictates spaces after "," in param lists. I can reformat using the eclipse java formatter before finally merging to clean these all up. I'm sure I missed some.


ModelNode defaultValue = m.get("defaultValue");
Copy link
Contributor

Choose a reason for hiding this comment

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

do not hardcode "defaultValue" - use the constant in the Attribute class. need to use the Attribute class and resolve expressions. See "getString" usages as examples. Also, when using the Attributes constant, you'll also pick up the change where this should not be camel-case

if (defaultValue.getType()!= ModelType.UNDEFINED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial: java format dictates space around != - you have it after, but not before

String defVal = defaultValue.asString();
operationParam.setDefaultvalue(defVal);
}
ret.add(operationParam);
}
return ret;
}

private static Map<String, String> getMapFromString(ModelNode modelNode, OperationContext context,
SimpleAttributeDefinition attrib) throws OperationFailedException {
ModelNode value = attrib.resolveModelAttribute(context, modelNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ public class SubsystemParser implements XMLStreamConstants, XMLElementReader<Lis
.addChild(builder(DMROperationDefinition.INSTANCE)
.setXmlElementName(DMROperationDefinition.OPERATION)
.addAttributes(DMROperationAttributes.ATTRIBUTES)
.addChild(builder(DMROperationParamDefinition.INSTANCE)
.setXmlElementName(DMROperationParamDefinition.PARAM)
.addAttributes(DMROperationParamAttributes.ATTRIBUTES)
)
)
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package org.hawkular.agent.monitor.inventory;

import java.util.List;

/**
* Defines an operation that can be executed on the managed resource.
*
Expand Down Expand Up @@ -46,10 +48,15 @@ public final class Operation<L> extends NodeLocationProvider<L> {
* @param operationName the actual name of the operation as it is known to the actual resource being managed.
* This is the name that is used when telling the managed resource what operation to invoke.
* It may or may not be the same as <code>name</code>.
* @param params Additional params for this operation definition, e.g. coming from dmr describe-operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

"describe-operation?" I think you mean "operation-dmr"

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant :read-operation-description :-)

* Can be null.
*/
public Operation(ID id, Name name, L location, String operationName) {
public Operation(ID id, Name name, L location, String operationName, List<OperationParam> params) {
super(id, name, location);
this.operationName = operationName;
if (params!=null && !params.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial - spaces around !=

addProperty("params", params);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a constant for "params" here in the Operation class - make it private for now if we don't need it elsewhere.

}
}

/**
Expand Down
Loading