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

ids #56

Merged
merged 1 commit into from
Mar 2, 2023
Merged

ids #56

merged 1 commit into from
Mar 2, 2023

Conversation

nicolasparada
Copy link
Contributor

@nicolasparada nicolasparada commented Feb 28, 2023

Add method to list plugin IDs. This is usefull for autocompletion on the CLI.

Addresses part of: https://github.com/calyptia/go-fluentbit-config/issues/55

@patrick-stephens
Copy link
Contributor

By ID is this the (super unsafe - fun times with non-determinism in config maps for me!) tail.0 style IDs?

@pwhelan
Copy link
Contributor

pwhelan commented Feb 28, 2023

By ID is this the (super unsafe - fun times with non-determinism in config maps for me!) tail.0 style IDs?

I also agree... I think we should add some level of determinism by aliasing (adding an alias property) to inputs by name in the cli and/or core-ui before we try and actually linked to this sort of thing in the CLI. I think it might even be OK to autogenerate aliases with what we guesstimate their ID to be.

Where is it going to be used @nicolasparada?

@nicolasparada
Copy link
Contributor Author

nicolasparada commented Feb 28, 2023

@pwhelan there is a function in Cloud that can make use of it: https://github.com/calyptia/cloud/blob/536435cb62556ffb2bf7ddba770acec50d47d388/config_parser.go#L471 (for trace sessions).

Also in the CLI was a TODO to change using the name of the plugin, for the ID: https://github.com/calyptia/cli/blob/dba6ed005df11bfe17d6c5cd6a8b3f7c3b6f5d13/completer/completer.go#L286 (for trace sessions too).

@nicolasparada
Copy link
Contributor Author

@patrick-stephens yes, check the tests.

@patrick-stephens
Copy link
Contributor

I think we should add some level of determinism by aliasing (adding an alias property)

Can you raise an issue for this @pwhelan ? It's a very important requirement I think - without aliases it's pretty useless I found for any large scale deployment as metrics cannot be relied on, alerts cannot be fired and it just is confusing between even pods sharing the same config map getting different ids.

Copy link

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

The code looks good.
Other than that I'd address remarks that Pat has

@patrick-stephens patrick-stephens merged commit f7722dc into main Mar 2, 2023
@patrick-stephens patrick-stephens deleted the ids branch March 2, 2023 11:06
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.

4 participants