Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CLEAN_ONLY flag #51

Merged
2 commits merged into from Sep 15, 2018
Merged

Add CLEAN_ONLY flag #51

2 commits merged into from Sep 15, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2018

When in an environment where there may be connectivity issues /tmp will be potentially filled by broken downloads. This happens as mktemp create new randomly named directories each time the script is run.

I know @qbit has issue #48 to remove some options but I felt this useful to add. I am not familiar with man page syntax so I did not add it there.

Thoughts? Perhaps it may be better to add cleaning of snap downloads in $DST or /tmp as the first step each time the script is run instead.

@qbit
Copy link
Owner

qbit commented Aug 30, 2018

Let me think on this one. I haven't been happy with the mktemp change I made.. specifically because of the random dirs it makes. It also breaks adding -C to ftp's options for re-starting. I currently solved this by setting DST in my conf..

I also tend to run snap, then copy the verified dist files around to other machines on my network.. so I don't really want to auto-remove after the run..

@ghost
Copy link
Author

ghost commented Aug 31, 2018

Currently the way I implemented it the cleaning happens on demand, the auto clean was just an idea. What you said makes sense though as to why it should not happen automatically. I did not see a -C flag which is why I added it but the letter can be changed to anything really.

@qbit
Copy link
Owner

qbit commented Sep 7, 2018

Yeah, the -C is specific to ftp.

So like I have this in my snap.conf:

FTP_OPTS:-C -V

Makes it so ftp resumes where it left off when a download fails.

@ghost
Copy link
Author

ghost commented Sep 9, 2018

Will that conflict? As far as I can tell with the -C that I added it exits after cleaning and never runs anything else. That should prevent it from conflicting with ftp. Perhaps when setting the variable it can jump to the function and ignore the other flags to ensure this behavior?

snap Outdated Show resolved Hide resolved
snap Show resolved Hide resolved
Copy link
Owner

@qbit qbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Wanna go ahead and merge it? Then I will sign everything.

@ghost ghost merged commit 84dcdc6 into qbit:master Sep 15, 2018
@ghost
Copy link
Author

ghost commented Sep 15, 2018

Merged, thanks for the review.

rm -rf $DST/snap.*
else
rm -rf /tmp/snap.*
exit 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed it before, but there was a missing fi here. Please make sure you run the script after making changes, just to be sure it works!

@ghost
Copy link
Author

ghost commented Sep 15, 2018

Not sure how I missed that either. Will fix ASAP and get another PR in.

@qbit
Copy link
Owner

qbit commented Sep 15, 2018

Not sure how I missed that either. Will fix ASAP and get another PR in.

No worries - I pushed a fix already (with slightly modified logic and comments explaining it :D)

@ghost
Copy link
Author

ghost commented Sep 15, 2018

Thanks. Will be extra diligent about testing going forward. Thanks for the explanation and updated logic.

@ghost ghost deleted the tmpclean branch September 23, 2018 23:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant