Page MenuHomePhabricator

Implement complex variable replacement DSLs in Harbormaster (application/json, application/xml)
Open, WishlistPublic

Description

Currently, you can only specify the method of an HTTP request for a Harbormaster Build Step, however some external build systems (such as TeamCity, which I'm targeting), require a post body that specifies build specifics instead of URL params. It would be nice to have a text box in an HTTP build step that a user could provide text of type application/xml or application/json and when the build is triggered by Phabricator, this body is sent with the HTTP POST.

I would also like to include the already provided injectable build variables into this POST body if the user deems it necessary.

Event Timeline

halojedi20 updated the task description. (Show Details)
halojedi20 added a project: Harbormaster.
halojedi20 renamed this task from Implement post request body text box to Implement POST request body text field.
halojedi20 added a subscriber: halojedi20.

Does adding variables with ?var=value to the URL definitely not work?

Correct, I've looked into hacking it with url params but it's a no go.

Oh, you need application/json or other weird stuff.

See also T9456.

So, @epriestley, I'm actually building a Harbormaster build plugin for TeamCity as we speak and this is one of the few things left I need. I've completely spec'd and am about 80% finished with the plugin on the TeamCity side.

I think this is generally reasonable, but this is the tricky part:

I would also like to include the already provided injectable build variables into this POST body if the user deems it necessary.

I think we should support this, and we can't do this naively. I think we have to write "JSON + Variables" and "XML + Variables" parsers and build parse trees for stuff like this to work:

{
  "key": ${value}
}

We could do it like this instead, but I think this is generally fairly awkward and undesirable (that is, the behavior seems very surprising to me):

{
  "key": "${value}"
}

See some related discussion in T8975.

This is trickier in XML because the correct way to escape the value depends on where it appears:

<${value} />
<tag ${value} />
<tag attr="${value}" />
<tag>${value}</tag>

This seems like a very difficult problem to me.

A possible alternative is to use a dedicated "Run TeamCity Build" step, similar to the "Run CircleCI Build" step from T9456. (If that stuff lands, writing this build step would get a bit easier since it pulled some of the logging/HTTP request stuff up a level.)

You can definitely do this in a hard-coded way to get things working for your use case (just copy the existing "MakeHTTPRequest" build step into phabricator/src/extensions/, then rename it and adjust the content type / payload -- the CircleCI build step in T9456 / D14286 has an example). This might not be very general but could potentially get things working for now until we write a "JSON + Variables" parser.

kaya added a subscriber: kaya.Nov 25 2015, 11:15 PM
epriestley renamed this task from Implement POST request body text field to Implement complex variable replacement DSLs in Harbormaster (application/json, application/xml).Nov 30 2015, 4:48 PM
epriestley triaged this task as Wishlist priority.
epriestley moved this task from Backlog to Far Future on the Harbormaster board.
eadler added a project: Restricted Project.Aug 3 2016, 5:31 PM

@eadler, does your use case need a fancy content type like application/json, or just plain old www-url-normal-web-form-encoded?

eadler added a comment.Aug 3 2016, 5:56 PM

plain old www-url-normal-web-form-encoded

Ah, that one's easier.

This is also likely easy to build as an extension (e.g., "Run build with X"), although we don't have a great template for it right now -- HarbormasterCircleCIBuildStepImplementation and HarbormasterHTTPRequestBuildStepImplementation are both sort of halfway toward what you probably need, and I think getFieldSpecifications() may get replaced with something more EditEngine-flavored in the future.

Are you targeting a custom system, or some widely deployed system?

eadler added a comment.Aug 3 2016, 6:03 PM

We're targeting jenkins. We just need to kick off a jenkins job.

I'm not super familiar with Jenkins -- what makes this job require parameters in the body? Other users seem to be using Jenkins successfully without requiring this capability on the Phabricator side, as far as I know.

eadler added a comment.Aug 3 2016, 6:14 PM

I suspect that others don't need parameters in the kickoff process? Let me look into it. Maybe we have not explored Jenkins in detail.

I think this is a perfectly reasonable capability to want in the general case, I just want to make sure that this isn't a big fancy solution to a problem that could also be solved by checking a checkbox or making a one-line change to the build or whatever. Definitely possible that most Jenkins users are just doing very basic stuff with it, though.

eadler added a comment.Aug 3 2016, 6:26 PM

after a deeper look it seems there might be an option to use a token for this. Let me play around with this a bit more

eadler added a comment.Aug 3 2016, 6:30 PM

yeah, there is an option to enable building via a specific URL and a GET request. Cool

eadler added a comment.Aug 3 2016, 6:36 PM

this relies on the "run with token as root" extension (https://wiki.jenkins-ci.org/display/JENKINS/Build+Token+Root+Plugin) which mostly bypasses other authentication mechanisms. This isn't that much of a problem in our environment, but is clearly non-optimal

avivey added a subscriber: avivey.Aug 3 2016, 6:47 PM

In my jenkins setup, I wanted to keep auth, so I did this:

  • Disable all the Build With Tokens options
  • Enabled Jenkins SSH-CLI plugin (Which lets you SSH into it and run commands)
  • Wrote my own "start a build" command, which is also returns enough information to keep track of the build (Queue ID, which the http-invoke method does)
  • Wrote a JenkinsSSHBuildStepImplementation that uses that build via SSH, and has a funny JSON field for the arguments.

The ssh command uses -Pparam=value CLI arguments.

It's all at least a little rough and not very pretty, but I can clean up most of it.

epriestley added a subscriber: kszatan.

(T12812 wants custom headers, but the only use case appears to be better covered by T12813.)

Itms added a subscriber: Itms.Feb 7 2018, 8:30 AM

(T12812 wants custom headers, but the only use case appears to be better covered by T12813.)

T12812 would also be beneficial for Jenkins users. The Token Root Plugin works but is an issue for our environment, as we would like to expose our Jenkins instance to the outside world. A better solution for us would be to use HTTP authentication, but Jenkins's CSRF Protection requires us to send an extra header.

cmmata added a subscriber: cmmata.Aug 31 2018, 10:26 AM