-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Rule: Fix temporary rule filename composition issue #4468
Merged
GiedriusS
merged 4 commits into
thanos-io:main
from
matej-g:fix-rules-manager-filenames
Jul 29, 2021
Merged
Rule: Fix temporary rule filename composition issue #4468
GiedriusS
merged 4 commits into
thanos-io:main
from
matej-g:fix-rules-manager-filenames
Jul 29, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Matej Gera <[email protected]>
matej-g
changed the title
rule: Fix rules filename composition issue
rule: Fix temporary rules filename composition issue
Jul 22, 2021
matej-g
changed the title
rule: Fix temporary rules filename composition issue
Rule: Fix temporary rules filename composition issue
Jul 22, 2021
matej-g
changed the title
Rule: Fix temporary rules filename composition issue
Rule: Fix temporary rule filename composition issue
Jul 22, 2021
Signed-off-by: Matej Gera <[email protected]>
matej-g
force-pushed
the
fix-rules-manager-filenames
branch
from
July 22, 2021 07:57
5fc0c5b
to
5a12db9
Compare
wiardvanrij
previously approved these changes
Jul 28, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could rebase this from main, that would be great. LGTM, thank you
GiedriusS
previously approved these changes
Jul 29, 2021
LGTM, I have fixed the conflicts! |
GiedriusS
previously approved these changes
Jul 29, 2021
Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
approved these changes
Jul 29, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Matej Gera [email protected]
Changes
Provides a fix for current issue with rule filenames, which are being written into a
.tmp-rules
directory for further processing. As was discovered in the linked issue, the current implementation could lead to temporary rule filenames being composed in a way, where trimming of the filename leads to unexpected result, i.e. theTrimLeft
function can in some cases trim the filename in a way which results in duplication of temporary rule filenames. This in turn manifests itself as some rules being 'ignored' by the rule component.Since trimming the filename does not seem to have the desired effect, the fix proposed here is to simply leave the full filename as is, which practically leads to same result but without possible name duplication issue.
Fixes #4232
Verification
Related tests have been updated to assess the fix is working; also tested manually with local run of
thanos rule