PR#2348: backend: allow devel instance to remove access files - copr/copr

copr / copr

#2348 backend: allow devel instance to remove access files

Merged 2 years ago by praiskup. Opened 2 years ago by frostyx.

copr/ frostyx/copr hitcounter-remove-dev-files into main

backend: allow devel instance to remove access files

Jakub Kadlcik • 2 years ago

e24081a

backend/run/copr-aws-s3-hitcounter

file modified

+4 -7

		`@@ -84,13 +84,10 @@`
		`"""`
		`Delete a file from AWS s3 bucket`
		`"""`
		`- # Refusing to delete anything from development instances. We don't have`
		`- # separate S3 buckets / directories for production and devel so removing`
		`- # a file on a devel instance would mean a data loss for production.`
		`- # We will remove files only from production and make devel instances`
		`- # count them incorrectly multiple times.`
		`- if gethostname() != PRODUCTION_HOSTNAME:`
		`- log.debug("Not deleting %s on a dev instance", s3file)`
		`+ # Refusing to delete anything from elsewhere than production or devel`
		`+ # instance (e.g. from docker container on our laptops)`
		`+ if gethostname() not in [PRODUCTION_HOSTNAME, DEVEL_HOSTNAME]:`
		`+ log.debug("Not deleting %s on this instance", s3file)`
praiskup commented 2 years ago Hmmm, without a bigger context - it looks like (a) I am a devel instance so (b) I am the DEVEL_HOSTNAME so (c) I'm going to remove the file. How do I know if the `s3file` is related to the devel instance?
praiskup commented 2 years ago Hm, I see the `delete_file()` method is not even called if it is not matching the current `gethostname()`. I'm curious whether we actually need this check then.
		`return`
		`if self.dry_run:`
		`return`

frostyx commented 2 years ago

We need to remove them so they don't waste space. I think I prefer
them to be removed by the production instance. There is a small danger
that it will remove something unexpected but I think it is better than
letting dev instance clean it for itself (dev could be broken and make
bigger mess).

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 06s

praiskup commented 2 years ago

I think it is better than letting dev instance clean it for itself

Hmm, there's a small risk, but I think I'd still actually prefer the other variant. I somewhat dislike that the production instance takes care of the development one... a precedent that I think is not worth it.

frostyx commented 2 years ago

Ok, no problem. I will change it.

rebased onto e24081a

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 57s

frostyx commented 2 years ago

PTAL

praiskup commented on line 15 of backend/run/copr-aws-s3-hitcounter 2 years ago

Hmmm, without a bigger context - it looks like (a) I am a devel instance so (b) I am the DEVEL_HOSTNAME so (c) I'm going to remove the file. How do I know if the s3file is related to the devel instance?

praiskup commented on line 15 of backend/run/copr-aws-s3-hitcounter 2 years ago

Hm, I see the delete_file() method is not even called if it is not matching the current gethostname(). I'm curious whether we actually need this check then.

praiskup commented 2 years ago