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

Issue #6008 - Allow absolute paths to be provided in start.ini for request log directory. #6009

Closed

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Feb 25, 2021

Closes #6008

Adds a new attribute in the .ini file called jetty.requestlog.absoluteFilePath.

This can be configured to provide an absolute path for the requestlog directory without changing the xml.

…quest log directory.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

@lachlan-roberts I know this PR is just following other precedent. But it is SO ugly! I really don't want this pattern to spread through our code.

Surely there must be a way we can resolve jetty.requestlog.dir against a base dir and work out if it is relative or absolute. Let's hangout about this.

…tra comments.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime
Copy link
Contributor

joakime commented Feb 25, 2021

We have too many ways to configure the same thing, a path. (in this case, we have .. absolute, relative-complete, and relative-parent)

If we only supported 2 modes (relative and absolute), AND if we started to expand properties found in XML attributes this could go from ...

<Property name="jetty.requestlog.absoluteFilePath">
  <Default>
    <Property name="jetty.base" default="." />/<Property name="jetty.requestlog.filePath">
      <Default><Property name="jetty.requestlog.dir" default="logs"/>/yyyy_mm_dd.request.log</Default>
    </Property>
  </Default>
</Property>

to

<Property name="jetty.requestlog.absoluteFilePath" default="${jetty.base}/${jetty.requestlog.relativeDir}/yyyy_mm_dd.request.log" />

@lachlan-roberts
Copy link
Contributor Author

@joakime we need to maintain support for the existing properties. If we change to your suggestion of

<Property name="jetty.requestlog.absoluteFilePath" default="${jetty.base}/${jetty.requestlog.relativeDir}/yyyy_mm_dd.request.log" />

then we will break anyone using the existing properties jetty.requestlog.filePath and jetty.requestlog.dir.

@joakime
Copy link
Contributor

joakime commented Mar 2, 2021

Draft PR #6022 is an alternate way to accomplish this.

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

@lachlan-roberts let's hold this in favor of #6022.

@joakime
Copy link
Contributor

joakime commented Mar 3, 2021

PR #6022 has been merged.
We can probably close this PR as obsolete.

@lachlan-roberts
Copy link
Contributor Author

This is replaced by #6022 and #6033.

@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-6008-requestLogAbsolutePath branch March 4, 2021 00:26
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.

Allow absolute paths to be provided in start.ini for request log directory.
4 participants