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

Jump to bottom

Append a suffix to temporary downloaded geoip files #16704

Merged
merged 3 commits into from Dec 10, 2020
Merged

Append a suffix to temporary downloaded geoip files #16704

merged 3 commits into from Dec 10, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Nov 11, 2020 •
edited by diosmosis

Description:

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl added the Needs Review PRs that need a code review label Nov 11, 2020
@diosmosis
Copy link
Member

Seems like it might be a bit of work, but is it possible to build this into the Http class so anyone that downloads to a file will have the suffix?

@sgiehl
Copy link
Member Author

sgiehl commented Nov 17, 2020

@diosmosis does it make sense to do that always? It would actually be a breaking change as downloaded files would suddenly have another name as expected - unless we make that optional, but then it would be kind of useless as the default would need to be to not append anything to have bc.

@diosmosis
Copy link
Member

@sgiehl good point, can't think of a way for it to just automatically work for existing uses.

@sgiehl sgiehl added this to the 4.1.0 milestone Nov 20, 2020
diosmosis
diosmosis approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

tested locally + code looks good

@sgiehl sgiehl merged commit cd0e0fe into 4.x-dev Dec 10, 2020
@sgiehl sgiehl deleted the downloadsuffix branch December 10, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sgiehl @diosmosis