This series brings a few enhancements to the tag releasing process. Patches 1 and 2 ensure that a released tag is unreleased before being deleted. Patch 3 lets the "release" cli command return an error when no publish server is configured, to allow quick detection of a misconfiguration. Patches 4 and 5 robustify tag (un)releasing. Julien Floret (5): tag: unrelease during auto cleanup tag: forbid deletion if released tag: return HTTP error to release if no server configured tag: fix unrelease when not found on publish server tag: leave unreleased if no job published Changes in v2: Increase pylint max-attributes limit (patch 1). Move "force=True" parameter removal from patch 1 to patch 2. dlrepo-cli | 8 ---- dlrepo/__main__.py | 1 + dlrepo/fs/__init__.py | 7 +++- dlrepo/fs/branch.py | 8 ++-- dlrepo/fs/tag.py | 88 ++++++++++++++++++++++++----------------- dlrepo/views/branch.py | 3 +- dlrepo/views/tag.py | 5 ++- docs/dlrepo-api.7.scdoc | 14 +++---- docs/dlrepo-cli.1.scdoc | 3 -- pylintrc | 2 +- 10 files changed, 73 insertions(+), 66 deletions(-) -- 2.30.2
dlrepo/patches/.build.yml: SUCCESS in 1m36s [enhance tag releasing][0] v2 from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40165 [1]: mailto:julien.floret@6wind.com ✓ #968118 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/968118
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~rjarry/dlrepo/patches/40165/mbox | git am -3Learn more about email & git
In the tag cleanup task, before deleting a released tag, make sure to remove it from the public server before. Indeed, we do not want tags hanging on the public mirror if they have been deleted from the main server. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/__main__.py | 1 + dlrepo/fs/__init__.py | 5 +++++ pylintrc | 2 +- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlrepo/__main__.py b/dlrepo/__main__.py index 0ece6e24817a..2767044c2275 100644 --- a/dlrepo/__main__.py +++ b/dlrepo/__main__.py @@ -39,6 +39,7 @@ async def create_semaphore(app: web.Application): # -------------------------------------------------------------------------------------- async def init_fs_cleanup(app: web.Application): repo = app["dlrepo_artifact_repository"] + repo.set_publish_semaphore(app["dlrepo_publish_semaphore"]) repo.start_cleanup_timer() loop = asyncio.get_running_loop() loop.add_signal_handler(signal.SIGUSR1, repo.schedule_cleanup_all) diff --git a/dlrepo/fs/__init__.py b/dlrepo/fs/__init__.py index d67856fc5f45..d74108970c08 100644 --- a/dlrepo/fs/__init__.py +++ b/dlrepo/fs/__init__.py @@ -94,6 +94,7 @@ class ArtifactRepository(AbstractRepository): self.cleanup_timer = None self.need_cleanup = False self.cleanup_tags = False + self.publish_semaphore = None def get_user_repo(self, user: str) -> "UserRepository": user = user.lower() @@ -126,6 +127,9 @@ class ArtifactRepository(AbstractRepository): def schedule_cleanup_all(self): self._schedule_cleanup(tags=True) + def set_publish_semaphore(self, semaphore: asyncio.Semaphore): + self.publish_semaphore = semaphore + def _schedule_cleanup(self, *, tags: bool): self.cleanup_tags = self.cleanup_tags or tags if self.cleanup_task: @@ -173,6 +177,7 @@ class ArtifactRepository(AbstractRepository): if isinstance(max_released, int) and max_released > 0: for tag in released_tags[max_released:]: LOG.info("Deleting released tag %s/%s ...", branch.name, tag.name) + await tag.do_release(False, self.publish_semaphore) await loop.run_in_executor(None, tag.delete, force=True) deleted += 1 diff --git a/pylintrc b/pylintrc index a93fe541e230..344348eba83b 100644 --- a/pylintrc +++ b/pylintrc @@ -52,7 +52,7 @@ expected-line-ending-format=LF [DESIGN] max-args=7 -max-attributes=7 +max-attributes=10 max-bool-expr=5 max-branches=20 max-locals=16 -- 2.30.2
We never want to delete a released tag from the main server, because we would then have a dangling tag published on the public mirror, with no easy way to manage it. To ensure it does not happen, remove the possibility to "force" deleting a released tag. The tag should be unreleased before. The "-f/--force" parameter of the "delete" cli command is kept for compatibility. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo-cli | 8 -------- dlrepo/fs/__init__.py | 2 +- dlrepo/fs/branch.py | 8 ++++---- dlrepo/fs/tag.py | 16 ++++++---------- dlrepo/views/branch.py | 3 +-- dlrepo/views/tag.py | 3 +-- docs/dlrepo-api.7.scdoc | 14 ++++++-------- docs/dlrepo-cli.1.scdoc | 3 --- 8 files changed, 19 insertions(+), 38 deletions(-) diff --git a/dlrepo-cli b/dlrepo-cli index 545269df93d9..6a5d4bc8cdde 100755 --- a/dlrepo-cli +++ b/dlrepo-cli @@ -483,12 +483,6 @@ def internal(args): Arg("branch", metavar="BRANCH", help="the branch name"), Arg("tag", metavar="TAG", nargs="?", help="the tag name"), Arg("job", metavar="JOB", nargs="?", help="the job name"), - Arg( - "-f", - "--force", - action="store_true", - help="force the deletion of 'released' tags", - ), ) def delete(args): """ @@ -500,8 +494,6 @@ def delete(args): if args.job: url = os.path.join("branches", args.branch, args.tag, args.job) + "/" else: - if args.force: - params["force"] = "true" url = os.path.join("branches", args.branch, args.tag) + "/" else: url = os.path.join("branches", args.branch) + "/" diff --git a/dlrepo/fs/__init__.py b/dlrepo/fs/__init__.py index d74108970c08..9d98b124349d 100644 --- a/dlrepo/fs/__init__.py +++ b/dlrepo/fs/__init__.py @@ -178,7 +178,7 @@ class ArtifactRepository(AbstractRepository): for tag in released_tags[max_released:]: LOG.info("Deleting released tag %s/%s ...", branch.name, tag.name) await tag.do_release(False, self.publish_semaphore) - await loop.run_in_executor(None, tag.delete, force=True) + await loop.run_in_executor(None, tag.delete) deleted += 1 LOG.info("Deleted %d tags", deleted) diff --git a/dlrepo/fs/branch.py b/dlrepo/fs/branch.py index a88cf786d1fd..8b6940c19d62 100644 --- a/dlrepo/fs/branch.py +++ b/dlrepo/fs/branch.py @@ -70,14 +70,14 @@ class Branch(SubDir): policy[field] = 0 return policy - def delete(self, *, force: bool = False): + def delete(self): if not self.exists(): raise FileNotFoundError() for t in self.get_tags(): if t.is_locked(): raise OSError(f"Tag {t.name} is locked") - if not force and t.is_released(): - raise OSError(f"Tag {t.name} is released, use force") + if t.is_released(): + raise OSError(f"Tag {t.name} is released, unrelease it first") for t in self.get_tags(): - t.delete(force=force) + t.delete() self.root().rmtree(self._path) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index b1bdac233550..098824b185b9 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -135,7 +135,7 @@ class Tag(SubDir): self.PUBLISH_URL, ) async with semaphore: - await sess.delete(self.url(), params={"force": "true"}) + await sess.delete(self.url()) p = self._publish_status_path() if p.is_file(): p.unlink() @@ -160,9 +160,7 @@ class Tag(SubDir): if job.is_internal(): # in case job was already published, delete it from the public server async with semaphore: - await sess.delete( - job_url, params={"force": "true"}, raise_for_status=False - ) + await sess.delete(job_url, raise_for_status=False) job.set_released(False) continue async with semaphore: @@ -179,9 +177,7 @@ class Tag(SubDir): continue # delete the job previously released before re-uploading it async with semaphore: - await sess.delete( - job_url, params={"force": "true"}, raise_for_status=False - ) + await sess.delete(job_url, raise_for_status=False) job.set_released(False) self._publish_status_path().write_text(f"uploading {job.name}\n") tasks = [] @@ -254,13 +250,13 @@ class Tag(SubDir): elif path.is_file(): path.unlink() - def delete(self, *, force: bool = False): + def delete(self): if not self.exists(): raise FileNotFoundError() if self.is_locked(): raise OSError(f"Tag {self.name} is locked") - if not force and self.is_released(): - raise OSError(f"Tag {self.name} is released, use force") + if self.is_released(): + raise OSError(f"Tag {self.name} is released, unrelease it first") for j in self.get_jobs(): j.delete() self.root().rmtree(self._path) diff --git a/dlrepo/views/branch.py b/dlrepo/views/branch.py index 720ba24437f4..90ccfaebcab3 100644 --- a/dlrepo/views/branch.py +++ b/dlrepo/views/branch.py @@ -127,8 +127,7 @@ class BranchView(BaseView): loop = asyncio.get_running_loop() branch = self._get_branch() try: - force = "force" in self.request.query - await loop.run_in_executor(None, lambda: branch.delete(force=force)) + await loop.run_in_executor(None, branch.delete) self.repo().schedule_cleanup_orphans() return web.Response() except OSError as e: diff --git a/dlrepo/views/tag.py b/dlrepo/views/tag.py index 5e75d28dc486..1fcb286c6778 100644 --- a/dlrepo/views/tag.py +++ b/dlrepo/views/tag.py @@ -113,8 +113,7 @@ class TagView(BaseView): loop = asyncio.get_running_loop() try: tag = self._get_tag() - force = "force" in self.request.query - await loop.run_in_executor(None, lambda: tag.delete(force=force)) + await loop.run_in_executor(None, tag.delete) self.repo().schedule_cleanup_orphans() return web.Response() except OSError as e: diff --git a/docs/dlrepo-api.7.scdoc b/docs/dlrepo-api.7.scdoc index 1d46bea6c8b4..8b0364cc9033 100644 --- a/docs/dlrepo-api.7.scdoc +++ b/docs/dlrepo-api.7.scdoc @@ -189,16 +189,15 @@ Change the cleanup policy of the specified _{branch}_. - _400_: invalid parameters in the request body. - _404_: the specified _{branch}_ does not exist. -## DELETE /branches/{branch}/?force -## DELETE /~{user}/branches/{branch}/?force +## DELETE /branches/{branch}/ +## DELETE /~{user}/branches/{branch}/ Delete the specified _{branch}_ and all its tags. *Access:* _rw_ *Errors:* - - _400_: if one of the tags is released or locked and _force_ was not - specified in the query parameters. + - _400_: if one of the tags is released or locked. - _404_: the specified _{branch}_ does not exist. # TAGS @@ -281,16 +280,15 @@ un-publication) of the tag to/from another server if *DLREPO_PUBLISH_URL* and The publication status can be fetched with a *GET* request on the same tag. -## DELETE /branches/{branch}/{tag}/?force -## DELETE /~{user}/branches/{branch}/{tag}/?force +## DELETE /branches/{branch}/{tag}/ +## DELETE /~{user}/branches/{branch}/{tag}/ Delete the specified _{tag}_ and all its jobs. *Access:* _rw_ *Errors:* - - _400_: the tag is locked or it is released and _force_ was not - specified in the query parameters. + - _400_: the tag is locked or released. - _404_: the specified _{tag}_ does not exist. - _405_: _{tag}_ is either _latest_, _stable_ or _oldstable_. diff --git a/docs/dlrepo-cli.1.scdoc b/docs/dlrepo-cli.1.scdoc index ef25de413a79..7cf8bf7e0f4a 100644 --- a/docs/dlrepo-cli.1.scdoc +++ b/docs/dlrepo-cli.1.scdoc @@ -205,9 +205,6 @@ _JOB_ Delete a job, a tag or a branch and all its tags recursively. -*-f*, *--force* - Force the deletion of _released_ tags. _locked_ tags cannot be deleted. - _BRANCH_ The branch name. _TAG_ -- 2.30.2
Before this patch, a tag could be set to "released" even if no publish server was configured. This is quite misleading, because the release has no effect. Let's return an HTTPMisdirectedRequest error to the client instead. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 49 ++++++++++++++++++++++++--------------------- dlrepo/views/tag.py | 2 ++ 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index 098824b185b9..c1736fa10c1d 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -68,6 +68,10 @@ class Tag(SubDir): return self._released_path().is_file() def set_released(self, released: bool, semaphore: asyncio.Semaphore): + if not self.PUBLISH_URL: + raise ValueError("DLREPO_PUBLISH_URL is undefined") + if not self.PUBLISH_AUTH: + raise ValueError("DLREPO_PUBLISH_AUTH is undefined") if not self._path.is_dir(): raise FileNotFoundError() loop = asyncio.get_running_loop() @@ -116,29 +120,28 @@ class Tag(SubDir): ) async def do_release(self, released: bool, semaphore: asyncio.Semaphore): - if self.PUBLISH_URL and self.PUBLISH_AUTH: - self._publish_status_path().write_text("in progress\n") - async with self._publish_session() as sess: - if released: - LOG.info( - "publishing tag %s/%s to %s", - self.parent.name, - self.name, - self.PUBLISH_URL, - ) - await self._publish(sess, semaphore) - else: - LOG.info( - "deleting tag %s/%s from %s", - self.parent.name, - self.name, - self.PUBLISH_URL, - ) - async with semaphore: - await sess.delete(self.url()) - p = self._publish_status_path() - if p.is_file(): - p.unlink() + self._publish_status_path().write_text("in progress\n") + async with self._publish_session() as sess: + if released: + LOG.info( + "publishing tag %s/%s to %s", + self.parent.name, + self.name, + self.PUBLISH_URL, + ) + await self._publish(sess, semaphore) + else: + LOG.info( + "deleting tag %s/%s from %s", + self.parent.name, + self.name, + self.PUBLISH_URL, + ) + async with semaphore: + await sess.delete(self.url()) + p = self._publish_status_path() + if p.is_file(): + p.unlink() path = self._released_path() if released: path.touch() diff --git a/dlrepo/views/tag.py b/dlrepo/views/tag.py index 1fcb286c6778..25657d8ad178 100644 --- a/dlrepo/views/tag.py +++ b/dlrepo/views/tag.py @@ -103,6 +103,8 @@ class TagView(BaseView): tag.set_stable(stable) except FileNotFoundError as e: raise web.HTTPNotFound() from e + except ValueError as e: + raise web.HTTPMisdirectedRequest(reason=str(e)) from e return web.Response() -- 2.30.2
If a tag flagged as "released" is not found on the publish server, just let the unrelease command remove the "release" flag. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index c1736fa10c1d..759f9afaa11d 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -138,7 +138,11 @@ class Tag(SubDir): self.PUBLISH_URL, ) async with semaphore: - await sess.delete(self.url()) + try: + await sess.delete(self.url()) + except aiohttp.client_exceptions.ClientResponseError as e: + if e.status != 404: + raise p = self._publish_status_path() if p.is_file(): p.unlink() -- 2.30.2
When releasing a tag, there is a possibility that no job is actually published, for example when all jobs are "internal". In this case, it is misleading to see the tag as "released", although it is absent from the publish server. After the release process, we issue a GET request to the public tag's URL. If the tag does not exist or contains no job, we remove the "released" state. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index 759f9afaa11d..1378f7549d15 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -120,6 +120,7 @@ class Tag(SubDir): ) async def do_release(self, released: bool, semaphore: asyncio.Semaphore): + published_jobs = [] self._publish_status_path().write_text("in progress\n") async with self._publish_session() as sess: if released: @@ -130,6 +131,15 @@ class Tag(SubDir): self.PUBLISH_URL, ) await self._publish(sess, semaphore) + async with semaphore: + try: + resp = await sess.get(self.url()) + data = await resp.read() + data = json.loads(data.decode("utf-8")) + published_jobs = data["tag"]["jobs"] + except aiohttp.client_exceptions.ClientResponseError as e: + if e.status != 404: + raise else: LOG.info( "deleting tag %s/%s from %s", @@ -143,16 +153,17 @@ class Tag(SubDir): except aiohttp.client_exceptions.ClientResponseError as e: if e.status != 404: raise - p = self._publish_status_path() - if p.is_file(): - p.unlink() - path = self._released_path() - if released: - path.touch() - elif path.is_file(): - path.unlink() + released_path = self._released_path() + if released and published_jobs: + released_path.touch() + else: + if released_path.is_file(): + released_path.unlink() for job in self.get_jobs(): job.set_released(False) + status_path = self._publish_status_path() + if status_path.is_file(): + status_path.unlink() async def _publish(self, sess: aiohttp.ClientSession, semaphore: asyncio.Semaphore): loop = asyncio.get_running_loop() -- 2.30.2
builds.sr.ht <builds@sr.ht>dlrepo/patches/.build.yml: SUCCESS in 1m36s [enhance tag releasing][0] v2 from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40165 [1]: mailto:julien.floret@6wind.com ✓ #968118 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/968118