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

Use os.TempDir() for temporary directory in default path #158

Merged

Conversation

Enrico204
Copy link
Contributor

@Enrico204 Enrico204 commented Aug 23, 2021

What is the purpose of this change? What does it change?

This pull request removes the hardcoded reference to /tmp/restic replacing with platform-specific path, retrieved using os.TempDir() stdlib function.

Was the change discussed in an issue or in the forum before?

Issue #157

Checklist

  • I have enabled maintainer edits for this PR
  • I have added tests for all changes in this PR
    • No need for test, os.TempDir() is a builtin function
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@wojas
Copy link
Contributor

wojas commented Aug 23, 2021

As mentioned in #157, we plan to remove the default path in a future release and make it a required argument.

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

@wojas: Do you have objections to cleanup the default path calculation for now until we're ready to change how the rest-server is configured?

cmd/rest-server/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
changelog/unreleased/pull-158 Outdated Show resolved Hide resolved
@wojas
Copy link
Contributor

wojas commented Sep 2, 2021

@wojas: Do you have objections to cleanup the default path calculation for now until we're ready to change how the rest-server is configured?

No, go ahead. It's just that having a default that eats your backups is not great anyway.

@Enrico204
Copy link
Contributor Author

I changed the code according to the review (except for the comment on the command line help section) :-)

@Enrico204 Enrico204 force-pushed the use-os-tempdir-for-temporary-directory branch from a933653 to 40bb1eb Compare September 6, 2021 20:43
@Enrico204
Copy link
Contributor Author

I've rebased this pull request to the current master to resolve a conflict :-)

@MichaelEischer MichaelEischer force-pushed the use-os-tempdir-for-temporary-directory branch from 40bb1eb to f952bc7 Compare September 12, 2021 19:27
Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

LGTM. I've rebased the branch to the current master, squashed the commits and replaced "O.S." with "OS" ;-)

@MichaelEischer MichaelEischer merged commit 9f8c31b into restic:master Sep 12, 2021
@Enrico204 Enrico204 deleted the use-os-tempdir-for-temporary-directory branch September 12, 2021 20:49
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.

3 participants