Closed Bug 508417 Opened 15 years ago Closed 14 years ago

pushsnip should ask for confirmation

Categories

(Release Engineering :: General, defect, P4)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: catlee, Assigned: catlee)

References

Details

(Whiteboard: [automation][updates][oldbug])

Attachments

(2 files, 1 obsolete file)

pushsnip should show you a list of files that are going to be updated, possibly with diffs, and then ask you for confirmation before actually pushing the snippets live. This should help prevent accidentally pushing the wrong snippets!
Sounds awesome! In the future!
Component: Release Engineering → Release Engineering: Future
Mass move of bugs from Release Engineering:Future -> Release Engineering. See http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering
Priority: -- → P3
Priority: P3 → P5
Whiteboard: [automation][updates]
Assignee: nobody → catlee
Whiteboard: [automation][updates] → [automation][updates][oldbug]
Priority: P5 → P4
Attachment #471841 - Flags: review?(nrthomas)
Attachment #471842 - Flags: review?(nrthomas)
Comment on attachment 471841 [details] [diff] [review] Import of pushsnip into tools repo, with prompt for confirmation >diff --git a/stage/pushsnip b/stage/pushsnip >+if test -z $snippetDir; then >+ echo Usage: $0 [snippet-directory-to-sync-in from $STAGING_DIR] Please add documentation for the -f mode. >+# Strip out slashes from snippetDir, to make sure it can be used >+# as a directory name >+newSnippetDir=`echo $snippetDir | $SED -e 's,/,,g'` I see you're not changing the intent of this line, but the point of it eludes me. It's more wide-reaching than removing a trailing backslash, and we only ever call pushsnip with a string that doesn't contain internal /'s. Do you grok it ? >+if [ $force = 0 ]; then Was expecting to see -eq instead of =, and 'man bash' isn't telling me why that's not an assignment operation. How does that work ? >+ $RSYNC -nPaO $STAGING_DIR/$newSnippetDir/ $LIVE_SNIPPET_DIR What response/checking do you expect when the output of this is hundreds of screens long ? eg 3.5.x/3.6.x pushing to beta/release. Is the point to catch pushing snippets from the wrong branch ? There won't be any cues to catch pushing snippets from an earlier build with this, which is the recent failure case. What scope of problems are you trying to prevent ? How much long does it take to do the rsync -n for a long-lived branch ? If it's ~10 minutes to test, and 10 more to push, then I wouldn't be surprised if people start 'priming the pump' before the driver's go and leave it at the prompt. In that case it's fairly dangerous to continue on only an enter (oops, wrong window!) and it would be better to prompt for a string like 'yes'.
Attachment #471842 - Flags: review?(nrthomas) → review+
(In reply to comment #5) > Comment on attachment 471841 [details] [diff] [review] > Import of pushsnip into tools repo, with prompt for confirmation > > >diff --git a/stage/pushsnip b/stage/pushsnip > >+if test -z $snippetDir; then > >+ echo Usage: $0 [snippet-directory-to-sync-in from $STAGING_DIR] > > Please add documentation for the -f mode. Added. > >+# Strip out slashes from snippetDir, to make sure it can be used > >+# as a directory name > >+newSnippetDir=`echo $snippetDir | $SED -e 's,/,,g'` > > I see you're not changing the intent of this line, but the point of it eludes > me. It's more wide-reaching than removing a trailing backslash, and we only > ever call pushsnip with a string that doesn't contain internal /'s. Do you grok > it ? It didn't make sense to me either. The only thing I could think of was maybe at one point we had names that contained slashes in them, and wanted to convert them to filesystem names? The current version of the patch just uses `basename`, which also deals with trailing slashes. > > >+if [ $force = 0 ]; then > > Was expecting to see -eq instead of =, and 'man bash' isn't telling me why > that's not an assignment operation. How does that work ? [ is an alias for test, and test supports = and != for string comparisons. > > >+ $RSYNC -nPaO $STAGING_DIR/$newSnippetDir/ $LIVE_SNIPPET_DIR > > What response/checking do you expect when the output of this is hundreds of > screens long ? eg 3.5.x/3.6.x pushing to beta/release. Is the point to catch > pushing snippets from the wrong branch ? There won't be any cues to catch > pushing snippets from an earlier build with this, which is the recent failure > case. What scope of problems are you trying to prevent ? I changed this to run a diff. It ignores files that only exist in the live snippet dir (so aren't being modified by the new snippets). > How much long does it take to do the rsync -n for a long-lived branch ? If it's > ~10 minutes to test, and 10 more to push, then I wouldn't be surprised if > people start 'priming the pump' before the driver's go and leave it at the > prompt. In that case it's fairly dangerous to continue on only an enter (oops, > wrong window!) and it would be better to prompt for a string like 'yes'. Added a check that the person enters 'yes'.
Attachment #471841 - Attachment is obsolete: true
Attachment #472781 - Flags: review?(nrthomas)
Attachment #471841 - Flags: review?(nrthomas)
So in the very long diff this will produce, we should be looking for a plausible build=BUILDID change ?
(In reply to comment #7) > So in the very long diff this will produce, we should be looking for a > plausible build=BUILDID change ? Yeah....do you have any suggestions for making the output better? What do you want to see before you push?
Probably not any good ones, it's hard to collapse the information from thousands of text files in a concise way. The only thing I can suggest is based on what I do every time I go to use pushsnip, which is to 'ls -lt | head -30' in /opt/aus2/snippets/staging and eyeball the list for more recent directories than I was expecting to push.
Comment on attachment 472781 [details] [diff] [review] Import of pushsnip into tools repo, with prompt for confirmation Lets give it a try and see how people react.
Attachment #472781 - Flags: review?(nrthomas) → review+
WONTFIXING in favour of bug 594930.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: