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

fix(sourceprocessor): fix templating for sourceCategoryPrefix #1350

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

aboguszewski-sumo
Copy link
Contributor

@aboguszewski-sumo aboguszewski-sumo commented Nov 28, 2023

Fixes #1322 - please see the issue to see the reason why it was reopened.
related to #1339

It turns out that the previous fix was not enough, because the code was affected also in some other places. I also did a small refactor.

@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner November 28, 2023 16:52
@github-actions github-actions bot added the go label Nov 28, 2023
@aboguszewski-sumo aboguszewski-sumo enabled auto-merge (rebase) November 28, 2023 16:55
}
valueTemplate = prefix + valueTemplate

if doesUseAnnotation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it matter if we use annotations or not? Should prefix templating only work when using annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question to the author of the processor.
As I understand it: if we use the annotations, then we override the standard template value or the prefix, and then we can't use the preprocessed template attributes array.

Copy link
Contributor

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Let's get the questions answered before we merge this.

Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

LGTM
Waiting for @astencel-sumo

@aboguszewski-sumo
Copy link
Contributor Author

@astencel-sumo

Copy link
Contributor

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Ideally let's fix that test, other than that 🚀

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-source-category-prefix-fix branch 2 times, most recently from 90fafcd to d425f78 Compare December 8, 2023 14:13
@github-actions github-actions bot removed the go label Dec 8, 2023
@aboguszewski-sumo
Copy link
Contributor Author

@astencel-sumo I think that I added that test because there was no test that just checks the prefixing (I had some problems while implementing this and this test helped me).
I just added your test, I guess it should be fine.

@aboguszewski-sumo aboguszewski-sumo enabled auto-merge (rebase) December 8, 2023 14:16
@aboguszewski-sumo aboguszewski-sumo merged commit 78541ae into main Dec 8, 2023
28 checks passed
@aboguszewski-sumo aboguszewski-sumo deleted the ab-source-category-prefix-fix branch December 8, 2023 14:41
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.

[sourceprocessor] sourceCategoryPrefix is not subject to templating
3 participants