Closed Bug 361297 Opened 13 years ago Closed 13 years ago

invalid steps not handled correctly

Categories

(Release Engineering :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

Attachments

(1 file, 2 obsolete files)

Specifying an invalid step name to "release -o" (run only this step) or "release -s" (start at this step) currently has the same effect as not specifying a step. The correct behavior should be to print an error and exit.
Blocks: end2end-bld
Attached patch handle missing step properly (obsolete) — Splinter Review
($stepNumber > scalar(@allSteps)) will never be true here.
Attachment #246040 - Flags: review?(preed)
Comment on attachment 246040 [details] [diff] [review]
handle missing step properly

My preference in areas such as these is to assert() validity early, and then when your preconditions are met, then do the necessary logic.

There's also the interesting bit that the loop will continue to run, even if the step has been found. Granted, this probably isn't too big a deal, but if there were a million steps, maybe it would be.

I'm also confused as to why the initialization of currentStep got moved.

I'd probably do something like (pseudocode):

if (!not grep(/step/, @validSteps) {
  print "Barf early.";
}

my $stepNumber = -1;

for($i = 0; $i < scalar(@steps); $i++) { # let the loop take care of the incrementing logic
  if (stepMatches) {
     $stepNumber = $i;
     last;
  }
}

if ($stepNumber == -1) {
   print "Ugh."
}
Assignee: build → rhelmer
Comment on attachment 246040 [details] [diff] [review]
handle missing step properly

this is better in the latest version, but I like the assertion idea. I'll submit another one for this later.
Attachment #246040 - Flags: review?(preed) → review-
Attachment #246040 - Attachment is obsolete: true
Attachment #259051 - Flags: review?(preed)
Comment on attachment 259051 [details] [diff] [review]
fail early if the step name is not valid


>+        if (not grep(/$desiredStep/, @allSteps)) {

I think you want
  if not grep(/^$desiredStep$/, @allSteps)) {
Attachment #259051 - Flags: review?(preed) → review-
Comment on attachment 259259 [details] [diff] [review]
address concerns from previous patch, also remove duplicate $currentStep declaration

>+            if ($allSteps[$i] eq "$desiredStep") {

Nit: $desiredStep does not need to be quoted.

Otherwise, looks ok.
Attachment #259259 - Flags: review?(preed) → review+
Landed, with nit fixed, thanks!

Checking in release;
/cvsroot/mozilla/tools/release/release,v  <--  release
new revision: 1.8; previous revision: 1.7
done
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.