Cleanup files.PublishedStorage after dropping a publish

[aol-nnov]

Fixes: #198

After dropping files.PublishedStorage there were some leftovers - empty directory tree. If that directory tree is empty, it is now removed.

Checklist

• functional test added/updated (if change is functional)

[aol-nnov]

@neolynx here is another annoying thing I'd like to fix in aptly. My publish prefix is bloated with empty directories as I have to create and remove publishes intensively.

[neolynx]

oh, yes please ! I found this quite annoying as well...

[aol-nnov]

yay, there is an issue for it! #198

[aol-nnov]

@neolynx original tests that were failing are now fixed, but my newly added PublishDrop21Test is failing, not sure why...

May be it's time for me to learn something new, too? 😁 This alienish test suite is driving me nuts, frankly...

[aol-nnov]

All tests are passing now

[neolynx]

@neolynx original tests that were failing are now fixed

great ! could you explain why they failed ?

but my newly added PublishDrop21Test is failing, not sure why...

May be it's time for me to learn something new, too? 😁 This alienish test suite is driving me nuts, frankly...

please do not judge :) the history of all of this is in git, many people worked on that. I think the thing to learn here is how to deal with legacy code and how to slowly improve it and revive the project. so please be respectful and constructive, we are all aware of the state of things, and it took months to get it to a working and halfway reliable state.

[aol-nnov]

could you explain why they failed ?

-gold file was missing for my PublishDrop21Test - copied it from PublishDrop2Test. Not completely sure what it is.

Whole experience of adding tests is cumbersome and non-obvious due to lack of developers documentation. Even some doc links to the testing frameworks would make the whole process more friendly and smooth...

Oh, and original tests were failing because checks relied on empty directories now removed (as you said, the history of all of this is in git :-) )

[neolynx]

could you explain why they failed ?

-gold file was missing for my PublishDrop21Test - copied it from PublishDrop2Test. Not completely sure what it is.

ok great. in the Makefile there is the CAPTURE flag, you can uncomment, then the tests will create the gold files.

Whole experience of adding tests is cumbersome and non-obvious due to lack of developers documentation. Even some doc links to the testing frameworks would make the whole process more friendly and smooth...

as said before, we are well aware of that. feel free to contribute. until then let's keep the discussions here technical.

Oh, and original tests were failing because checks relied on empty directories now removed (as you said, the history of all of this is in git :-) )

thanks, that is excellent :-)

[neolynx]

tests look good !

I started a coverage build: codecov/patch — 84.61% of diff hit (target 74.84%)

[aol-nnov]

Seems, ci is stuck again...

[neolynx]

Seems, ci is stuck again...

seems we have a segfault:
https://github.com/aptly-dev/aptly/actions/runs/11476603052/job/31936962068?pr=1381#step:8:487

[aol-nnov]

seems we have a segfault

Heh, that's because nil is passed instead of aptly.Progress in tests. Very handy 😒

P.S.: handled that on my side

[aol-nnov]

Now, seems -gold-en outputs do not match new reality 😂

[neolynx]

Now, seems -gold-en outputs do not match new reality 😂

it needs that non empty check before remove... ;-)

[aol-nnov]

Also, something wrong with docker tests:

$ make docker-image
[+] Building 310.3s (21/21) FINISHED
$ make docker-system-test
usermod: UID '0' already exists
make: *** [docker-system-test] Error 4
$ _

So, for me there is no simple way to update test now, unfortunately.

[aol-nnov]

it needs that non empty check before remove... ;-)

You know what, my experience contributing in this particular PR is drastically falling down. Second day in a row we're chewing through tons of comments in a changeset that barely changes nothing, docs on testsuite are missing, tooling does not work and you're making jokes on me...

That all really makes me sad.

[neolynx]

Also, something wrong with docker tests:

the docker-wrapper sets the user to the one of the git repo directory, to not write files as root. it seems your git dir is owned by root. changing to user owner would solve this in that case.

[aol-nnov]

it seems your git dir is owned by root

not true

$ ls -lad ../aptly/
drwxr-xr-x@ 46 aol  staff  1472 Oct 22 17:22 ../aptly/

Thanks for the docker-wrapper idea, though. Slashed it a bit.

[aol-nnov]

Re: better tooling #1384

It might be worth merging it first.

[aol-nnov]

A friendly ping for @neolynx

Tests are fixed now.

Let's finish those two PRs, we're on the homestretch ;)

[aol-nnov]

Rebased to the tip of master, conflicts resolved.

P.S.: Failing tests are not mine.

[neolynx]

we need to use normal logging here, no json to console...

[neolynx]

P.S.: Failing tests are not mine.

resources from the internet are not always available, I restarted the tests.

[neolynx]

Dear @aol-nnov,

I was by no means making jokes on you, i was merely a wink to point out what is expected here. Your skills and contributions are very welcome, also for documentation and all the other stuff which is lagging behind !

I talked to some of the other maintainers, here are our thougths:

• Aptly does not have the authority over the prefix path, as it is most likely used in a webserver directory structure, which might be managed by other tools
• Changing the default behavior to remove the tree could result in 404 instead of a empty directory index on the webserver
• Proper logging of unexpected error is required, as aptly logs are crucial for reproducing issues, also expected errors are prevented / not logged
• there is also deb/publish.go:func (p *PublishedRepo) RemoveFiles(publishedStorageProvider aptly.PublishedStorageProvider, removePrefix bool, - the removePrefix might be doing something similar, maybe this can be combined?
• Issue comments are marked as resolved by the maintainers. Even if the concern might feel unreasonable or not to your liking.

To move forward here, we suggest the following changes:

• make a cleanupTree argument to the drop calls (api, cmd), cleanup up the tree only if set to true and skipCleanup is false
• in CleanupPublishTree, check if the directory is empty and return if true, otherwise remove directory. return the error and log in the caller
• original test should prove that prefix is not removed, new tests prove that empty dirs are removed, non-empty ones not (without a error log)
• if we wanna be fancy, a test with a parent directory without write permissions would prove that the error is logged

Please understand that aptly is a legacy project with many maintainers and contributors over the years. It took considerable effort to revive it and bring it to the state it is now. As maintainers, our main concern is to not break any of potentially thousands of installations, and if something happens, we have proper logs. And this has some implications on how we can implement things.

@cfiehe, @szakalboss, @dario-gallucci please add if I forgot something