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

loki.source.journald: sync positions directory permissions #3973

Merged

Conversation

tpaschalis
Copy link
Member

@tpaschalis tpaschalis commented May 25, 2023

PR Description

This PR fixes the permissions which are used to create the positions directory for loki.source.journal to match all the other loki.source.* components. It also moves the directory creation to before the definition of the file.

Eg. here's how it works for loki.source.docker

err := os.MkdirAll(o.DataPath, 0750)
if err != nil && !os.IsExist(err) {
return nil, err
}
positionsFile, err := positions.New(o.Logger, positions.Config{
SyncPeriod: 10 * time.Second,
PositionsFile: filepath.Join(o.DataPath, "positions.yml"),
IgnoreInvalidYaml: false,
ReadOnly: false,

Which issue(s) this PR fixes

Fixes #3972

Notes to the Reviewer

Nothing in particular. Maybe this should also be part of the shared abstraction for logging components.

PR Checklist

  • CHANGELOG updated
  • Documentation added (N/A)
  • Tests updated (N/A)

Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@tpaschalis tpaschalis force-pushed the sync-loki-journal-positions-permissions branch from 9124ad0 to 18d3c90 Compare May 25, 2023 08:37
@tpaschalis tpaschalis marked this pull request as ready for review May 25, 2023 08:37
@tpaschalis tpaschalis requested review from a team as code owners May 25, 2023 08:37
@tpaschalis tpaschalis requested a review from thampiotr May 25, 2023 08:38
Copy link
Contributor

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

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

LGTM! Agreed that it should ideally be part of some common library or part of a wrapper around the positions structs, but it's higher priority to fix the issue first.

@tpaschalis tpaschalis merged commit 341b19c into grafana:main May 25, 2023
@tpaschalis tpaschalis deleted the sync-loki-journal-positions-permissions branch May 25, 2023 11:37
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flow: loki.source.journal error writing positions file - v0.33.2
2 participants