Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6646052:
Remove --client_prefix argument and check. (Closed)
|
Created:
9 years, 9 months ago by Greg Spencer (Chromium) Modified:
9 years, 6 months ago CC:
chromium-os-reviews_chromium.org, ericli, DaleCurtis, anush, Nick Sanders Visibility:
Public. |
DescriptionThis removes the --client_prefix argument from the dev server, because it's no longer necessary.
BUG=none
TEST=ran an update from a current dev image without any arguments. Ran devserver_test.py
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c8b59b2
Patch Set 1 #Patch Set 2 : Fixing test. #Patch Set 3 : Removing flag entirely. #
Total comments: 2
Patch Set 4 : Added deprecation warning #
Messages
Total messages: 15 (0 generated)
I've grepped through all the code I can find, and I don't see any references to MementoSoftwareUpdate anywhere, but perhaps I don't have some factory components. Before checking this in, it would be good to know that nothing in the factory flow depends on this default value. Is "update_test.py" even used anymore? I didn't modify that because it looks too old to matter.
This will break factory install. Zdenek has a plan to fork factory install devserver and developer devserver, but I'm not sure what the status is.
Yep, LBTM. The factory workflow still uses (and likely will keep using) Memento updater. There's a plan for dropping support for Memento updater from various related scripts (ie. payload generators), but so far we've only changed the default in those to --noold_style, and devserver will have to be forked (which is the plan because factory functionality is almost disjoint with everything else), but that is a secondary issue after moving factory scripts. On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: > This will break factory install. Zdenek has a plan to fork factory install > devserver and developer devserver, but I'm not sure what the status is. > > > http://codereview.chromium.org/6646052/ >
OK, any chance we can get the factory scripts to specify '--client_prefix MementoSoftwareUpdate' so that they explicitly use it instead of implicitly? This change only fixes the default value of that flag to a reasonable default for developers, it doesn't remove any support for memento updater. -Greg. On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> wrote: > Yep, LBTM. The factory workflow still uses (and likely will keep using) > Memento updater. There's a plan for dropping support for Memento updater > from various related scripts (ie. payload generators), but so far we've only > changed the default in those to --noold_style, and devserver will have to be > forked (which is the plan because factory functionality is almost disjoint > with everything else), but that is a secondary issue after moving factory > scripts. > > On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: > >> This will break factory install. Zdenek has a plan to fork factory install >> devserver and developer devserver, but I'm not sure what the status is. >> >> >> http://codereview.chromium.org/6646052/ >> > >
Do we even need the client prefix? Why not just remove it now and accept any requests that get sent? Systems shouldn't have both updaters on the system either way On Mon, Mar 14, 2011 at 8:25 AM, Greg Spencer <gspencer@chromium.org> wrote: > OK, any chance we can get the factory scripts to specify '--client_prefix > MementoSoftwareUpdate' so that they explicitly use it instead of implicitly? > This change only fixes the default value of that flag to a reasonable > default for developers, it doesn't remove any support for memento updater. > -Greg. > > On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> wrote: >> >> Yep, LBTM. The factory workflow still uses (and likely will keep using) >> Memento updater. There's a plan for dropping support for Memento updater >> from various related scripts (ie. payload generators), but so far we've only >> changed the default in those to --noold_style, and devserver will have to be >> forked (which is the plan because factory functionality is almost disjoint >> with everything else), but that is a secondary issue after moving factory >> scripts. >> On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: >>> >>> This will break factory install. Zdenek has a plan to fork factory >>> install >>> devserver and developer devserver, but I'm not sure what the status is. >>> >>> http://codereview.chromium.org/6646052/ >> > >
On Mon, Mar 14, 2011 at 11:53 AM, Chris Sosa <sosa@chromium.org> wrote: > Do we even need the client prefix? Why not just remove it now and > accept any requests that get sent? Systems shouldn't have both > updaters on the system either way > Yes -- it seems the prefix is unused now. Apparently it was meant to prevent the update server from serving updates to two different update engines on the same client. We should just remove it. Darin > > On Mon, Mar 14, 2011 at 8:25 AM, Greg Spencer <gspencer@chromium.org> > wrote: > > OK, any chance we can get the factory scripts to specify '--client_prefix > > MementoSoftwareUpdate' so that they explicitly use it instead of > implicitly? > > This change only fixes the default value of that flag to a reasonable > > default for developers, it doesn't remove any support for memento > updater. > > -Greg. > > > > On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> > wrote: > >> > >> Yep, LBTM. The factory workflow still uses (and likely will keep using) > >> Memento updater. There's a plan for dropping support for Memento updater > >> from various related scripts (ie. payload generators), but so far we've > only > >> changed the default in those to --noold_style, and devserver will have > to be > >> forked (which is the plan because factory functionality is almost > disjoint > >> with everything else), but that is a secondary issue after moving > factory > >> scripts. > >> On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: > >>> > >>> This will break factory install. Zdenek has a plan to fork factory > >>> install > >>> devserver and developer devserver, but I'm not sure what the status is. > >>> > >>> http://codereview.chromium.org/6646052/ > >> > > > > >
Cool. I'll change this CL into a CL removing the option entirely. -Greg. On Mon, Mar 14, 2011 at 11:57 AM, Darin Petkov <petkov@chromium.org> wrote: > On Mon, Mar 14, 2011 at 11:53 AM, Chris Sosa <sosa@chromium.org> wrote: > >> Do we even need the client prefix? Why not just remove it now and >> accept any requests that get sent? Systems shouldn't have both >> updaters on the system either way >> > > Yes -- it seems the prefix is unused now. Apparently it was meant to > prevent the update server from serving updates to two different update > engines on the same client. We should just remove it. > > Darin > > >> >> On Mon, Mar 14, 2011 at 8:25 AM, Greg Spencer <gspencer@chromium.org> >> wrote: >> > OK, any chance we can get the factory scripts to specify >> '--client_prefix >> > MementoSoftwareUpdate' so that they explicitly use it instead of >> implicitly? >> > This change only fixes the default value of that flag to a reasonable >> > default for developers, it doesn't remove any support for memento >> updater. >> > -Greg. >> > >> > On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> >> wrote: >> >> >> >> Yep, LBTM. The factory workflow still uses (and likely will keep using) >> >> Memento updater. There's a plan for dropping support for Memento >> updater >> >> from various related scripts (ie. payload generators), but so far we've >> only >> >> changed the default in those to --noold_style, and devserver will have >> to be >> >> forked (which is the plan because factory functionality is almost >> disjoint >> >> with everything else), but that is a secondary issue after moving >> factory >> >> scripts. >> >> On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: >> >>> >> >>> This will break factory install. Zdenek has a plan to fork factory >> >>> install >> >>> devserver and developer devserver, but I'm not sure what the status >> is. >> >>> >> >>> http://codereview.chromium.org/6646052/ >> >> >> > >> > >> > >
Also, please send me, Ryan, adlr or petkov the review for this since we've worked the most on this code. No offense to Anush, but he's never actually changed the code and is inappropriate as a reviewer. On Mon, Mar 14, 2011 at 12:03 PM, Greg Spencer <gspencer@chromium.org> wrote: > Cool. I'll change this CL into a CL removing the option entirely. > -Greg. > > On Mon, Mar 14, 2011 at 11:57 AM, Darin Petkov <petkov@chromium.org> wrote: >> >> On Mon, Mar 14, 2011 at 11:53 AM, Chris Sosa <sosa@chromium.org> wrote: >>> >>> Do we even need the client prefix? Why not just remove it now and >>> accept any requests that get sent? Systems shouldn't have both >>> updaters on the system either way >> >> Yes -- it seems the prefix is unused now. Apparently it was meant to >> prevent the update server from serving updates to two different update >> engines on the same client. We should just remove it. >> Darin >> >>> >>> On Mon, Mar 14, 2011 at 8:25 AM, Greg Spencer <gspencer@chromium.org> >>> wrote: >>> > OK, any chance we can get the factory scripts to specify >>> > '--client_prefix >>> > MementoSoftwareUpdate' so that they explicitly use it instead of >>> > implicitly? >>> > This change only fixes the default value of that flag to a reasonable >>> > default for developers, it doesn't remove any support for memento >>> > updater. >>> > -Greg. >>> > >>> > On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> >>> > wrote: >>> >> >>> >> Yep, LBTM. The factory workflow still uses (and likely will keep >>> >> using) >>> >> Memento updater. There's a plan for dropping support for Memento >>> >> updater >>> >> from various related scripts (ie. payload generators), but so far >>> >> we've only >>> >> changed the default in those to --noold_style, and devserver will have >>> >> to be >>> >> forked (which is the plan because factory functionality is almost >>> >> disjoint >>> >> with everything else), but that is a secondary issue after moving >>> >> factory >>> >> scripts. >>> >> On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: >>> >>> >>> >>> This will break factory install. Zdenek has a plan to fork factory >>> >>> install >>> >>> devserver and developer devserver, but I'm not sure what the status >>> >>> is. >>> >>> >>> >>> http://codereview.chromium.org/6646052/ >>> >> >>> > >>> > >> > >
Sorry I meant to say ... Anush doesn't seem to be a good reviewer for this CL rather than inappropriate. Kudos to petkov for making fun of me for using that word :p On Mon, Mar 14, 2011 at 12:06 PM, Chris Sosa <sosa@chromium.org> wrote: > Also, please send me, Ryan, adlr or petkov the review for this since > we've worked the most on this code. No offense to Anush, but he's > never actually changed the code and is inappropriate as a reviewer. > > On Mon, Mar 14, 2011 at 12:03 PM, Greg Spencer <gspencer@chromium.org> wrote: >> Cool. I'll change this CL into a CL removing the option entirely. >> -Greg. >> >> On Mon, Mar 14, 2011 at 11:57 AM, Darin Petkov <petkov@chromium.org> wrote: >>> >>> On Mon, Mar 14, 2011 at 11:53 AM, Chris Sosa <sosa@chromium.org> wrote: >>>> >>>> Do we even need the client prefix? Why not just remove it now and >>>> accept any requests that get sent? Systems shouldn't have both >>>> updaters on the system either way >>> >>> Yes -- it seems the prefix is unused now. Apparently it was meant to >>> prevent the update server from serving updates to two different update >>> engines on the same client. We should just remove it. >>> Darin >>> >>>> >>>> On Mon, Mar 14, 2011 at 8:25 AM, Greg Spencer <gspencer@chromium.org> >>>> wrote: >>>> > OK, any chance we can get the factory scripts to specify >>>> > '--client_prefix >>>> > MementoSoftwareUpdate' so that they explicitly use it instead of >>>> > implicitly? >>>> > This change only fixes the default value of that flag to a reasonable >>>> > default for developers, it doesn't remove any support for memento >>>> > updater. >>>> > -Greg. >>>> > >>>> > On Sat, Mar 12, 2011 at 2:07 AM, Zdenek Behan <zbehan@chromium.org> >>>> > wrote: >>>> >> >>>> >> Yep, LBTM. The factory workflow still uses (and likely will keep >>>> >> using) >>>> >> Memento updater. There's a plan for dropping support for Memento >>>> >> updater >>>> >> from various related scripts (ie. payload generators), but so far >>>> >> we've only >>>> >> changed the default in those to --noold_style, and devserver will have >>>> >> to be >>>> >> forked (which is the plan because factory functionality is almost >>>> >> disjoint >>>> >> with everything else), but that is a secondary issue after moving >>>> >> factory >>>> >> scripts. >>>> >> On Sat, Mar 12, 2011 at 7:33 AM, <nsanders@chromium.org> wrote: >>>> >>> >>>> >>> This will break factory install. Zdenek has a plan to fork factory >>>> >>> install >>>> >>> devserver and developer devserver, but I'm not sure what the status >>>> >>> is. >>>> >>> >>>> >>> http://codereview.chromium.org/6646052/ >>>> >> >>>> > >>>> > >>> >> >> >
On Mon, Mar 14, 2011 at 12:09 PM, Chris Sosa <sosa@chromium.org> wrote: > On Mon, Mar 14, 2011 at 12:06 PM, Chris Sosa <sosa@chromium.org> wrote: > > Also, please send me, Ryan, adlr or petkov the review for this since > > we've worked the most on this code. No offense to Anush, but he's > > never actually changed the code and is inappropriate as a reviewer. > Sorry I meant to say ... Anush doesn't seem to be a good reviewer for > this CL rather than inappropriate. Kudos to petkov for making fun of > me for using that word :p Will do. So I guess that was an inappropriate use of the word "inappropriate"? :) -Greg.
OK, reviewers, please take another look. I've removed the client prefix check.
LGTM
LGTM as well. Thanks for fixing the assert http://codereview.chromium.org/6646052/diff/10001/devserver.py File devserver.py (right): http://codereview.chromium.org/6646052/diff/10001/devserver.py#newcode121 devserver.py:121: parser.add_option('--client_prefix', dest='client_prefix_deprecated', Maybe instead of in the help ... print out something later that prints out a fat warning that says to change your script.
http://codereview.chromium.org/6646052/diff/10001/devserver.py File devserver.py (right): http://codereview.chromium.org/6646052/diff/10001/devserver.py#newcode121 devserver.py:121: parser.add_option('--client_prefix', dest='client_prefix_deprecated', On 2011/03/15 20:33:49, sosa wrote: > Maybe instead of in the help ... print out something later that prints out a fat > warning that says to change your script. OK, added this.
Thanks! On Tue, Mar 15, 2011 at 2:14 PM, <gspencer@chromium.org> wrote: > > http://codereview.chromium.org/6646052/diff/10001/devserver.py > File devserver.py (right): > > http://codereview.chromium.org/6646052/diff/10001/devserver.py#newcode121 > devserver.py:121: parser.add_option('--client_prefix', > dest='client_prefix_deprecated', > On 2011/03/15 20:33:49, sosa wrote: >> >> Maybe instead of in the help ... print out something later that prints > > out a fat >> >> warning that says to change your script. > > OK, added this. > > http://codereview.chromium.org/6646052/ > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
