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

enhancement(kubernetes platform): add the ability to configure Vector API at the Helm charts #6248

Merged
merged 9 commits into from Jan 28, 2021

Conversation

dzirg44
Copy link
Contributor

@dzirg44 dzirg44 commented Jan 26, 2021

According to the #6227 I implemented helm api endpoint configuration.
I am not sure in the current implementation, because it looks a "little bit" weird ))
But I couldn't make up something better.
Also I am not so confident in grammar and will be glad to fix my mistakes.

Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44 dzirg44 requested review from a team January 26, 2021 19:51
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Thanks a lot for preparing a PR for us!

This implementation is too complicated, and I suggest we go with a more direct one - as I mention in the comment I left above.

Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 27, 2021

I made it simpler, but maybe we have to check if we have api vars or not?
or just add default variables to the ?
https://github.com/timberio/vector/blob/master/distribution/helm/vector-agent/values.yaml

@MOZGIII
Copy link
Contributor

MOZGIII commented Jan 27, 2021

Yeah, vector-agent should have the vars too. It makes sense, cause any vector instance can serve the API.

Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 27, 2021

fixed, also my linter found some wrong whitespaces here , i can delete or keep it.

Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 27, 2021

fixed, thank you for your patience )

@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 27, 2021

should I squash commits or you will?

@MOZGIII
Copy link
Contributor

MOZGIII commented Jan 27, 2021

I'll take care of the squash (in fact, Github will do it for us automatically).

Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Looks good, all that's left is making the checks pass.
You need to:

@MOZGIII MOZGIII added the ci-condition: k8s e2e tests enable Run Kubernetes E2E test suite for this PR label Jan 27, 2021
Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 27, 2021

Done.

@MOZGIII MOZGIII changed the title enhancement(helm chart): add built-in api endpoint enhancement(kubernetes platform): add the ability to configure Vector API at the Helm charts Jan 28, 2021
@MOZGIII
Copy link
Contributor

MOZGIII commented Jan 28, 2021

Great, checks are passing! I've left some final touches, and we can merge after they're fixed!
Thanks a lot for your contribution!

Signed-off-by: Oleg Tsymbal <dzirg44@gmail.com>
@dzirg44
Copy link
Contributor Author

dzirg44 commented Jan 28, 2021

Changed! Thank you for mentoring!

@dzirg44 dzirg44 requested review from MOZGIII and removed request for a team January 28, 2021 11:38
@MOZGIII MOZGIII merged commit e5ed779 into vectordotdev:master Jan 28, 2021
@MOZGIII
Copy link
Contributor

MOZGIII commented Jan 28, 2021

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-condition: k8s e2e tests enable Run Kubernetes E2E test suite for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants