add ability to edit existing tree

REOPENED
Assigned to

Status

Webtools Graveyard
Tinderbox
REOPENED
11 years ago
4 years ago

People

(Reporter: rhelmer, Assigned: reed)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
We can add a tree via admintree.cgi, but there is no way (as far as I can tell) to edit settings an existing tree, without modifying the files directly.

For tinderbox.mozilla.org, if anyone with sheriff access messes up tree creation then we need to escalate to IT, and I'd rather spare them :)
(Assignee)

Updated

11 years ago
Whiteboard: DUPEME?

Comment 1

11 years ago
I think that we would want a dependency upon bug 322418 as you don't want just anyone tweaking the tree settings.  On the other hand, we do allow anyone to create a tree.  If you don't care about anyone being able to change settings, it may be easiest to just add a 'delete tree' option for this case. :-)
Depends on: 322418

Comment 2

11 years ago
confused.

http://tinderbox.mozilla.org/showbuilds.cgi has an entire section devoted to editing trees, it includes links like this:

http://tinderbox.mozilla.org/admintree.cgi?tree=Mozilla-l10n-sv-SE

Comment 3

11 years ago
The current admintree page only lets you edit the tree display and trim logs.  There's no way to edit the tree configuration (ie, the contents of treedata.pl) except manually.
(Assignee)

Comment 4

10 years ago
Created attachment 320657 [details] [diff] [review]
patch - v1

Things this patch does:
* Adds an "Edit a tinderbox's configuration" section to admintree.cgi.
* Modifies create_tree() in doadmin.cgi to support both creating and modifying tree configs.
* Fixes regular expression to pass Perl warnings being thrown during testing.
* Changes some whitespace.

Remaining issue to think about:
* How best to deal with XSS attempts via tree config variables. We don't do any input checking or output encoding, but that's not really new to anything on admintree.cgi except the tree name, so I'm not sure I really care that much, considering you need the password to change the variables anyway. Thoughts?

It's live on http://tinderbox-stage.mozilla.org/ if you want to play.
Assignee: morgamic → reed
Status: NEW → ASSIGNED
Attachment #320657 - Flags: review?(cls)
Attachment #320657 - Flags: review?(bear)

Comment 5

10 years ago
Comment on attachment 320657 [details] [diff] [review]
patch - v1

The code looks fine but I don't like that the internal setup of the server is available for just anyone to see, much less have a chance of tweaking.  That information should only be available to system level admins.  It should be hidden behind another access mechanism, like how params are/were hidden in bugzilla.
Attachment #320657 - Flags: review?(cls) → review-

Comment 6

10 years ago
Comment on attachment 320657 [details] [diff] [review]
patch - v1

looks simple enough but it just hangs all the internal variables out for viewing.  +1 to cls' suggestion
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> (From update of attachment 320657 [details] [diff] [review])
> looks simple enough but it just hangs all the internal variables out for
> viewing.  +1 to cls' suggestion

True, but people can go to Bonsai and see the internal variables themselves... :)

However, would fixing bug 398050 suffice for you and cls?
(Assignee)

Updated

10 years ago
Whiteboard: DUPEME?
(Assignee)

Comment 8

10 years ago
Created attachment 322886 [details] [diff] [review]
patch - v2

Now that bug 398050 has landed, this was much easier. Does this work for you all?
Attachment #320657 - Attachment is obsolete: true
Attachment #322886 - Flags: review?(cls)
Attachment #322886 - Flags: review?(bear)
Attachment #320657 - Flags: review?(bear)
(Assignee)

Updated

10 years ago
Whiteboard: waiting on reviews

Updated

10 years ago
Attachment #322886 - Flags: review?(bear) → review+
(Assignee)

Comment 9

10 years ago
Checking in admintree.cgi;
/cvsroot/mozilla/webtools/tinderbox/admintree.cgi,v  <--  admintree.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in doadmin.cgi;
/cvsroot/mozilla/webtools/tinderbox/doadmin.cgi,v  <--  doadmin.cgi
new revision: 1.37; previous revision: 1.36
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
No longer depends on: 322418
Resolution: --- → FIXED
Whiteboard: waiting on reviews
(Assignee)

Updated

10 years ago
Depends on: 398050

Comment 10

10 years ago
Comment on attachment 322886 [details] [diff] [review]
patch - v2

Why are you alternating between using '$treedata->{field}' and '$::global_treedata->{tree}->{field}' ?
Attachment #322886 - Flags: review?(cls) → review-

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

10 years ago
(In reply to comment #10)
> (From update of attachment 322886 [details] [diff] [review])
> Why are you alternating between using '$treedata->{field}' and
> '$::global_treedata->{tree}->{field}' ?  

$treedata doesn't contain everything I need (see tb_load_data() in tbglobals.pl). I'm more than happy to add what I need to tb_load_data() and just use $treedata. That sound good to you?
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.