Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#115 closed enhancement (fixed)

WithProperties should be supported in most (all?) BuildStep arguments

Reported by: bhearsum Owned by:
Priority: major Milestone: 0.7.7
Version: 0.7.6 Keywords:
Cc: gward, dustin

Description

There's a few bugs on here about supporting WithProperties? on different BuildStep? arguments. I'd like to propose that BuildProperties? should be supported in most, if not all, BuildStep? arguments. I've come across times when they would be useful in mastersrc/slavedest for file transferring, workdir's (on more than just ShellCommand?), and custom, BuildStep? subclass arguments. I think it would be great if BuildStep? went through it's arguments and looked for WithProperties? (I don't know if this would be really expensive though). IMHO this shouldn't be something that subclasses need to implement.

Attachments (1)

ticket-115-patchset.zip (5.7 KB) - added by gward 13 years ago.
Patch series to implement this feature

Download all attachments as: .zip

Change History (6)

comment:1 Changed 13 years ago by gward

  • Cc gward added
  • Version set to 0.7.6

I'm working on a patch for this, at least in the context of build steps. (Note that at least one other ticket, #120, proposed using WithProperties? in another context, namely a status target. It seems odd for something in buildbot.status.* to depend on something in buildbot.steps.*, but maybe that's just me.)

The basic idea of my patch is to factor out a utility function render():

def render(s, build):
    """Return a string based on s and build that is suitable for use
    in a running BuildStep.  If s is a string, return s.  If s is a
    WithProperties object, return the result of s.render(build).
    Otherwise, return str(s).

That function, along with WithProperties? itself (and its helper class _BuildPropertyDictionary) would live in buildbot.process.buildstep. This seems like a sensible location if we want to support WithProperties? in any build step. If we want to support it anywhere period (e.g. status targets), then this is probably *not* the right place ... but I'll leave that decision up to Brian.

I'll post patches here soon, as soon as I'm happy with them.

Changed 13 years ago by gward

Patch series to implement this feature

comment:2 Changed 13 years ago by gward

The patch series I just attached does two things:

1) make it transparent to use WithProperties? in just about any BuildStep? context 2) modify two of those contexts -- specifically, ShellCommand?.describe() and the filenames

in FileUpload/FileDownload? -- to use WithProperties? transparently

The basic idea, as I said in my previous comment, is to factor out a function render() that takes any object (string, WithProperties?, whatever) and returns a string. (Actually, it also has to take a Build, so the WithProperties? has something to get properties from.) Patches 0001 .. 0004 implement this refactoring.

Then patch 0005 modifies ShellCommand?.describe() so it transparently supports WithProperties? by appropriate use of render(). Patch 0006 does the same thing for filenames in FileUpload? and FileDownload?.

comment:3 Changed 12 years ago by dustin

  • Cc dustin added

Patches are recorded in http://darcs.r.igoro.us/buildbot/dustin, in patches beginning with "#115:". It's available in the latest tarball in http://darcs.r.igoro.us/buildbot/dustin/dist.

comment:4 Changed 12 years ago by warner

  • Milestone changed from undecided to 0.7.7
  • Resolution set to fixed
  • Status changed from new to closed

Applied, in a series of patches ending with [5a45b4edb62d45ff263c8b3dfba6928d36a881d0].

Thanks!

-Brian

comment:5 Changed 12 years ago by bhearsum

Thanks so much for this patch Greg!

Note: See TracTickets for help on using tickets.