Notification end point data #968
Conversation
|
|
||
| private Map<String, Object> createMaterialDataMap(Material material) { | ||
| Map<String, Object> materialMap = new LinkedHashMap<String, Object>(); | ||
| if (material instanceof GitMaterial) { |
There was a problem hiding this comment.
All of these should be in their own classes. That's why polymorphic dispatch exists. :) I realize they don't all share an interface. Make them. Something like, InformationProvider, and all kinds of materials implement that interface and they decide what information to put in their own maps.
There was a problem hiding this comment.
will do once Go has feature-branch support :)
There was a problem hiding this comment.
There are already a couple of polymorphic attribute providers like this and this. I cannot reuse them cause they only populate fields that are part of identity. Adding another for this purpose looked like worsening the problem.
Also, the attributes of map cannot be changed much since any change would break the API. So there is very little chance of it being reused well.
I know it doesn't look great in the current form. Let me see how I can make this better.
There was a problem hiding this comment.
I think it's better to have them polymorphic and have strong tests (maybe at StageStatusPluginNotifierTest itself) to enforce API compatibility. Any change there should break a test here, showing that the API would change.
There was a problem hiding this comment.
I have made it polymorphic for materials with an interface InformationProvider.
…elinesqlmapdao instead.
| @@ -88,6 +88,10 @@ public MaterialConfig config() { | |||
| return new TfsMaterialConfig(url, userName, domain, getPassword(), projectPath, goCipher, autoUpdate, filter, folder, name); | |||
| } | |||
|
|
|||
| public String getDomain() { | |||
There was a problem hiding this comment.
If/when it becomes polymorphic (other comment here), you should remove this.
There was a problem hiding this comment.
i think this getter is useful. every materials' every field is exposed with a getter apart from TFS - domain & project-path.
|
This looks interesting... Is there more detail about what the actual change is going to look like? EDIT: heh, just saw @arvindsv's comment... |
|
@matt-richardson Yes, this is the discussion about that. There is going to be some more information available. I've asked @srinivasupadhya to put up a JSON of the new information that the Go Server will send, so that we can talk about it. |
|
Some thoughts on the sample xml:
My use case was this app - https://github.com/matt-richardson/gocd-windows-tray-build-notifier (which is about 10% complete) - which i would want to:
So, seems the xml snippet above seems pretty good for this use case (assuming the build cause is there). |
…Id since its cached. + populate data of relevant stage only
@matt-richardson yes. The password & every field of material configuration will be available to plugin. Its to enable plugin authors to use the data. use-case: i would like to update GitHub PR status (building,passed,failed). To do this I would would need the whole configuration (url, username, password). I was thinking of adding a warning message on plugins listing page saying
Yes. You should be able to achieve that with the available data. |
|
Yeah, I don't think the password should be there. Let the plugin author / users figure out another way to update that information. For instance, github provides API keys for such things. I don't think passwords should be provided. At the end of the day, a plugin runs inside the Go Server and can be malicious. I understand that, and probably there's nothing to stop a determined plugin from causing havoc, even if we do not provide the password. However, let's remember that this is a notification plugin endpoint and design it towards that. Not towards a specific use-case (GitHub PR status). |
|
From our experience using the Notification API, this covers several use cases - namely getting a list of materials that might have caused the stage failure and getting the job that failed the stage. We've also seen requests for a pipeline group property in addition to the pipeline name so we can more effectively support teams with multiple pipelines under the same group. Could we get that added as well? |
|
Yes, @ashwanthkumar has asked for it. I will add it for now. Lets see how the performance turns out. |
…s better, using that.
…xposed to notification plugins + material attributes should be provided by each material through polymorphism
|
I have updated the payload JSON with latest output. Have a look & let me know your thoughts - @matt-richardson, @pshelton-skype, @ashwanthkumar Done:
Also:
|
|
Im thinking of getting rid of "pipeline-name": "pipeline-name",
"pipeline-counter": "1",
"stage-name": "stage-name",
"stage-counter": "1",
"stage-state": "Passed",
"stage-result": "Passed"since its redundant now with |
|
I think I'm okay with it either way. It's easier to parse if I'm interested in this stage/status the first way, but not too much more difficult the proposed way. |
|
I would prefer (for my use case at least) to have all stages, not just the current one. If its not provided, I'll have to call the history api and get the stages from that. That said, I understand that you've got to balance load vs downstream use cases... |
…ad will be 'pipeline' & rest of the data can be accessed through it.
|
@pshelton-skype - I agree its easier. But the parsing from json to map will happen once. After that accessing data will be: responseMap.get("pipeline").get("name")
responseMap.get("pipeline").get("counter")
responseMap.get("pipeline").get("stage").get("name")
responseMap.get("pipeline").get("stage").get("counter")instead of: responseMap.get("pipeline-name")
responseMap.get("pipeline-counter")
responseMap.get("stage-name")
responseMap.get("stage-counter")looks ok. @matt-richardson - yes by having to add just the stage whose status changed I get do reduce queries to be run. Its just one DB query to get |
|
This is ready for final review. |
|
@srinivasupadhya Unfortunately, that would mean I have to add a database (or some method of persistance) to my client, which I didn't have to when I had all the stages :( |
|
@srinivasupadhya Is there a way get |
|
If your code looks like rather than you know something about using the |
|
@matt-richardson We're using the APIs extensively to collect more information on the stage, pipeline, and job, even with these changes. |
|
@pshelton-skype, do you have a repo/gist/description of how you are consuming the api? |
| for (MaterialRevision currentRevision : materialRevisions) { | ||
| Map<String, Object> materialRevisionMap = new LinkedHashMap<String, Object>(); | ||
|
|
||
| materialRevisionMap.put("material", ((InformationProvider) currentRevision.getMaterial()).getAttributes(false)); |
There was a problem hiding this comment.
Oh, come on, a small change like this makes the material a proper InformationProvider and doesn't need casts like this all over. You implemented the polymorphism change all over. Why wouldn't you just do this? It could even be marked abstract, since all the descendants of Material already implement it. Doing it this way just leads to someone seeing the Material interface, not implementing this method, and it'll fail with a ClassCastException at runtime, somewhere.
diff --git a/common/src/com/thoughtworks/go/config/materials/AbstractMaterial.java b/common/src/com/thoughtworks/go/config/materials/AbstractMaterial.java
index 9fe8fac..bfaef5c 100644
--- a/common/src/com/thoughtworks/go/config/materials/AbstractMaterial.java
+++ b/common/src/com/thoughtworks/go/config/materials/AbstractMaterial.java
@@ -182,4 +182,9 @@ public abstract class AbstractMaterial extends PersistentObject implements Mater
public MaterialConfig config() {
throw new RuntimeException("You need to implement this");
}
+
+ @Override
+ public Map<String, Object> getAttributes(boolean addSecureFields) {
+ return new HashMap<String, Object>();
+ }
}
diff --git a/common/src/com/thoughtworks/go/domain/materials/Material.java b/common/src/com/thoughtworks/go/domain/materials/Material.java
index 9138ddd..7c21fa5 100644
--- a/common/src/com/thoughtworks/go/domain/materials/Material.java
+++ b/common/src/com/thoughtworks/go/domain/materials/Material.java
@@ -16,10 +16,6 @@
package com.thoughtworks.go.domain.materials;
-import java.io.File;
-import java.io.Serializable;
-import java.util.Map;
-
import com.thoughtworks.go.config.CaseInsensitiveString;
import com.thoughtworks.go.config.PipelineConfig;
import com.thoughtworks.go.config.materials.SubprocessExecutionContext;
@@ -29,7 +25,11 @@ import com.thoughtworks.go.util.command.EnvironmentVariableContext;
import com.thoughtworks.go.util.command.ProcessOutputStreamConsumer;
import com.thoughtworks.go.util.json.JsonMap;
-public interface Material extends Serializable {
+import java.io.File;
+import java.io.Serializable;
+import java.util.Map;
+
+public interface Material extends Serializable, InformationProvider {
//-- DB and config behaviour
|
Sorry to get in the middle of your conversation, @matt-richardson and @pshelton-skype. :) Just want to let @srinivasupadhya know that I've looked at this and it looks good to me. Will wait on @srinivasupadhya to see if there's anything else missing (apart from a small |
|
@pshelton-skype - here is an example of how the data will be used. Edit: you are right, I haven't put the typecasting bit. Sorry about that. |
|
@arvindsv - there are a lot of implementors of |
|
I think an empty one in |
…thod in Material interface
|
@ashwanthkumar Edit: have done this. |
… to safegaurd against concurrent access & edit
|
Looks like we are done! |
|
Is there a plan to include pipeline/stage/job environment variables into the extension point? If they're already there how to gather them. Another question: Looking at the endpoint. There is an empty information about the modification's data outcome. Is that the list of SCM commits? |
done as part of #867
build-casefrom database &pipeline-groupfrom config.{ "pipeline": { "group": "pipeline-group", "name": "pipeline-name", "counter": "1", "build-cause": [ { "material": { "git-configuration": { "branch": "branch", "url": "http:\/\/user:******@gitrepo.com" }, "type": "git" }, "changed": true, "modifications": [ { "revision": "1", "modified-time": "2015-03-19T12:09:32.081Z", "data": { } } ] } ], "stage": { "name": "stage-name", "counter": "1", "state": "Passed", "result": "Passed", "create-time": "2011-07-13T19:43:37.100Z", "last-transition-time": "2011-07-13T19:43:37.100Z", "jobs": [ { "name": "job-name", "schedule-time": "2011-07-13T19:43:37.100Z", "complete-time": "2011-07-13T19:43:37.100Z", "state": "Completed", "result": "Passed", "agent-uuid": "uuid" } ] } } }sample JSON for all types of materials:
{ "material": { "git-configuration": { "branch": "branch", "url": "http:\/\/user:******@gitrepo.com" }, "type": "git" }, }, { "material": { "mercurial-configuration": { "url": "http:\/\/user:******@hgrepo.com" }, "type": "mercurial" }, }, { "material": { "svn-configuration": { "username": "username", "check-externals": false, "url": "http:\/\/user:******@svnrepo.com" }, "type": "svn" }, }, { "material": { "tfs-configuration": { "username": "username", "project-path": "project-path", "domain": "domain", "url": "http:\/\/user:******@tfsrepo.com" }, "type": "tfs" }, }, { "material": { "perforce-configuration": { "username": "username", "use-tickets": false, "view": "view", "url": "127.0.0.1:1666" }, "type": "perforce" }, }, { "material": { "pipeline-configuration": { "pipeline-name": "pipeline-name", "stage-name": "stage-name" }, "type": "pipeline" }, }, { "material": { "plugin-id": "pluginid", "package-configuration": { "k3": "package-v1" }, "type": "package", "repository-configuration": { "k1": "repo-v1" } }, }, { "material": { "plugin-id": "pluginid", "type": "scm", "scm-configuration": { "k1": "v1" } }, }