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

Added Default LogLabel provider with PropertiesAsLabels option #22

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

jincod
Copy link
Contributor

@jincod jincod commented Aug 23, 2020

No description provided.

@jincod jincod mentioned this pull request Aug 23, 2020
@jincod
Copy link
Contributor Author

jincod commented Aug 23, 2020

Hi @josephwoodward,

Thank you for your project!

@Falco20019
Copy link
Contributor

We are also looking forward to this :)

@jincod
Copy link
Contributor Author

jincod commented Oct 2, 2020

Hi @Falco20019

You can use this now

<?xml version="1.0" encoding="utf-8"?>
<configuration>
        <packageSources>
                <add key="local" value=".\libs" />
        </packageSources>
</configuration>

@Falco20019
Copy link
Contributor

@jincod Thanks :) I had a look at #15 which is partly overlapping but more flexible. I need some properties as labels (like SourceContext) which would not be possible with this approach. So I started my build based on that branch.

@josephwoodward
Copy link
Owner

josephwoodward commented Oct 2, 2020

@Falco20019 Interested to see what you come up with. I've been slowly working through #15 but as it's quite a large PR there's a lot changing in there and I need to get back up to speed.

I like this PR because it's smaller and doesn't change so much of the behaviour, but like you said, #15 is more flexible.

@josephwoodward
Copy link
Owner

I'm also thinking PropertiesAsLabels should be false by default to prevent an accidental explosion of labels which could prove an issue with high throughout systems. Instead you should have to explicitly opt in to the property being turned into a label so that such a change is visible in a PR instead of being unaware of it happening.

@Falco20019
Copy link
Contributor

Not sure if I would merge #15 and #22. Since #15 is introducing a flag for the strategy, this behavior can already be defined there. I think it might be still useful to be able to do it from the provider too, but it would need to be rebased once #15 is in since they both target the same area and would lead to conflicts.

@jincod
Copy link
Contributor Author

jincod commented Oct 5, 2020

@josephwoodward

I've done some refactoring and introduced PropertiesAsLabels as list of required labels

@josephwoodward josephwoodward merged commit d88dde0 into josephwoodward:master Nov 10, 2020
@Falco20019 Falco20019 mentioned this pull request Nov 23, 2020
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.

None yet

3 participants