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 instance name to download path when downloading files #1614

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vexx32
Copy link

@vexx32 vexx32 commented Jan 15, 2020

If the same download path is used for all instances, it may prove problematic, especially if it is configured to download specific files; the file would be overwritten and the data lost.

Adding the instance name to the path allows the feature to be used by multiple instances in succession without potentially overwriting files.

/cc @tas50 @smurawski @gep13

@vexx32
Copy link
Author

vexx32 commented Jan 15, 2020

I think that's all the tests passing... the rest of the CI appears to be failing much earlier in the chain, so I don't think it's related to my changes. Feel free to correct me if I'm wrong! 😊 💖

@vexx32 vexx32 requested a review from smurawski January 15, 2020 14:36
If the same download path is used for all instances, it may prove
problematic, especially if it is configured to download specific files;
the file would be overwritten.

Adding the instance name to the path allows the feature to be used by
multiple instances in succession without potentially overwriting files.
@vexx32 vexx32 force-pushed the instance-named-download-folders branch from 2cc03bb to 27e31c1 Compare January 22, 2020 13:57
@vexx32
Copy link
Author

vexx32 commented Jan 22, 2020

From what I can see from the logs, I don't think the CI error is related to this PR. Give me a ping if anything more is needed here. 🙂

@vexx32 vexx32 requested a review from fnichol January 28, 2020 17:29
@lamont-granquist lamont-granquist added the Breaking Change A known / planned breaking change to make label Feb 22, 2021
@lamont-granquist
Copy link
Contributor

this is probably the correct thing to do, but in the event someone has figured out this works fine for single instances they may be happily using it and this would break any scripts they have build wrapping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change A known / planned breaking change to make
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants