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

Switch log snapshot from file path reference to UUID #93

Merged
merged 3 commits into from
Sep 18, 2017

Conversation

AFaust
Copy link
Contributor

@AFaust AFaust commented Jul 23, 2017

CHECKLIST

  • There aren't existing pull requests attempting to address the issue mentioned here
  • Submission developed in a feature branch--not master

CONVINCING DESCRIPTION

This PR changes the way the log snapshot feature deals with identifying a particular snapshot session. Instead of using the full file path for reference (and having to validate it), a generic UUID is now being used. This avoids any encoding issues e.g. when the file path may contain backslashes instead of regular slashes.
In addition to this change, various smaller aspects have also been addressed:

  • move the temporary file handling into the temporary appender itself
  • move the appender layout default into the temporary appender itself
  • switch the snapshot start web script to use proper HTTP method (POST) - note that completion web script could not be switched to proper HTTP method as it is required to be a GET for use in the web UI

RELATED INFORMATION

Fixes #91

Note: By editing the Java-backed web scripts my formatter / auto-save rules have been applied to yield a consistent style. We still have #83 to further consolidate, document style conventions and potentially provide tools to achieve consistent code style.

- move temporary file handling into appender
- use correct HTTP method for snapshot creation web script
- deregister appender only after all validations succeeded
@AFaust AFaust merged commit 6b0731b into OrderOfTheBee:master Sep 18, 2017
@AFaust AFaust deleted the issue-91 branch September 18, 2017 17:19
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.

2 participants