Move service generated files to .old (fixes #845)

[lnussel]

fixes #845

[marcus-h]

On 2020-09-30 01:24:18 -0700, Ludwig Nussel wrote: diff --git a/osc/core.py b/osc/core.py index fd2c1a48..907256cf 100644 --- a/osc/core.py +++ b/osc/core.py @@ -415,11 +415,9 @@ def execute(self, dir, callmode = None, singleservice = None, verbose = None): # cleanup existing generated files for filename in os.listdir(dir): if filename.startswith('_service:') or filename.startswith('_service_'): - ent = os.path.join(dir, filename) - if os.path.isdir(ent): - shutil.rmtree(ent) - else: - os.unlink(ent) + if not os.path.exists('.old'): + os.mkdir('.old') + os.rename(filename, os.path.join('.old', filename))

Hmm the change makes sense but maybe we should error out if ".old" exists? I'm a bit hesitant to remove a dir, which could have been created by a user... What do you think?

[adrianschroeter]

On Mittwoch, 30. September 2020, 20:10:36 CEST Marcus Hüwe wrote: On 2020-09-30 01:24:18 -0700, Ludwig Nussel wrote: > diff --git a/osc/core.py b/osc/core.py > index fd2c1a48..907256cf 100644 > --- a/osc/core.py > +++ b/osc/core.py > @@ -415,11 +415,9 @@ def execute(self, dir, callmode = None, singleservice = None, verbose = None): > # cleanup existing generated files > for filename in os.listdir(dir): > if filename.startswith('_service:') or filename.startswith('_service_'): > - ent = os.path.join(dir, filename) > - if os.path.isdir(ent): > - shutil.rmtree(ent) > - else: > - os.unlink(ent) > + if not os.path.exists('.old'): > + os.mkdir('.old') > + os.rename(filename, os.path.join('.old', filename)) > Hmm the change makes sense but maybe we should error out if ".old" exists? I'm a bit hesitant to remove a dir, which could have been created by a user... What do you think?

A different name would be good to avoid conflict with user created dirs, but we would become incompatible with bs_service ... There is no other way then just to remove it IMHO...

…

-- Adrian Schroeter <adrian@suse.de> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

[lnussel]

I've updated the code to throw an exception if the directory was not created by the osc run itself

[marcus-h]

On 2020-09-30 22:51:18 -0700, Adrian Schröter wrote: On Mittwoch, 30. September 2020, 20:10:36 CEST Marcus Hüwe wrote:

<SNIP>

> Hmm the change makes sense but maybe we should error out if ".old" exists? > I'm a bit hesitant to remove a dir, which could have been created by a > user... What do you think? A different name would be good to avoid conflict with user created dirs, but we would become incompatible with bs_service ...

Ideally, the services would support an "--old-dir /path/to/old/dir" option.

[marcus-h]

Sigh... github ignores some mails again...

----- Forwarded message from Marcus Hüwe <suse-tux@gmx.de> -----

Date: Thu, 1 Oct 2020 13:46:06 +0200
From: Marcus Hüwe <suse-tux@gmx.de>
To: reply+AAIEUTBFAPXNRDYWOAO6AFV5QF7SREVBNHHCU3CO44@reply.github.com
Subject: Re: [openSUSE/osc] Move service generated files to .old (fixes #845)
 (#846)

On 2020-10-01 09:15:20 +0000, Ludwig Nussel wrote:
> I've updated the code to throw an exception if the directory was not created by the osc run itself
> 
Thanks! From a usability POV, we should make sure that we always cleanup
the old dir (for instance, if a service fails). What about the following?

diff --git a/osc/core.py b/osc/core.py
index fd2c1a4..f6d1e2c 100644
--- a/osc/core.py
+++ b/osc/core.py
@@ -410,16 +410,27 @@ class Serviceinfo:
return r

 def execute(self, dir, callmode = None, singleservice = None, verbose = None):

•    old_dir = os.path.join(dir, '.old')
  

•    if os.path.exists(old_dir) or os.path.islink(old_dir):
  

•        msg = '"%s" exists, please remove it' % old_dir
  

•        raise oscerr.OscIOError(None, msg)
  

•    try:
  

•        os.mkdir(old_dir)
  

•        return self._execute(dir, old_dir, callmode, singleservice,
  

•                             verbose)
  

•    finally:
  

•        if os.path.isdir(old_dir):
  

•            shutil.rmtree(old_dir)
  

• def _execute(self, dir, old_dir, callmode=None, singleservice=None,

•             verbose=None):
     import tempfile
  
     # cleanup existing generated files
     for filename in os.listdir(dir):
         if filename.startswith('_service:') or filename.startswith('_service_'):
  

•            ent = os.path.join(dir, filename)
  

•            if os.path.isdir(ent):
  

•                shutil.rmtree(ent)
  

•            else:
  

•                os.unlink(ent)
  

•            os.rename(os.path.join(dir, filename),
  

•                      os.path.join(old_dir, filename))
  
     allservices = self.services or []
     service_names = [s['name'] for s in allservices]
  

----- End forwarded message -----

[lnussel]

feel free to throw away my code and put your own :)

[marcus-h]

On 2020-10-01 09:15:20 +0000, Ludwig Nussel wrote: I've updated the code to throw an exception if the directory was not created by the osc run itself

Thanks! From a usability POV, we should make sure that we always cleanup the old dir (for instance, if a service fails). What about the following? ``` diff --git a/osc/core.py b/osc/core.py index fd2c1a4..f6d1e2c 100644 --- a/osc/core.py +++ b/osc/core.py @@ -410,16 +410,27 @@ class Serviceinfo: return r def execute(self, dir, callmode = None, singleservice = None, verbose = None): + old_dir = os.path.join(dir, '.old') + if os.path.exists(old_dir) or os.path.islink(old_dir): + msg = '"%s" exists, please remove it' % old_dir + raise oscerr.OscIOError(None, msg) + try: + os.mkdir(old_dir) + return self._execute(dir, old_dir, callmode, singleservice, + verbose) + finally: + if os.path.isdir(old_dir): + shutil.rmtree(old_dir) + + def _execute(self, dir, old_dir, callmode=None, singleservice=None, + verbose=None): import tempfile # cleanup existing generated files for filename in os.listdir(dir): if filename.startswith('_service:') or filename.startswith('_service_'): - ent = os.path.join(dir, filename) - if os.path.isdir(ent): - shutil.rmtree(ent) - else: - os.unlink(ent) + os.rename(os.path.join(dir, filename), + os.path.join(old_dir, filename)) allservices = self.services or [] service_names = [s['name'] for s in allservices] ```

[marcus-h]

On 2020-10-01 04:56:14 -0700, Ludwig Nussel wrote: feel free to throw away my code and put your own :)

I just created #847 PS: Sorry for being too stupid to post a patch via github... PPS: eventually, my original mail got through... (after a considerable delay:) )