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

Feat/optional-istio - Ignore the Istio process in the beginning #56

Merged
merged 4 commits into from
Aug 4, 2021
Merged

Feat/optional-istio - Ignore the Istio process in the beginning #56

merged 4 commits into from
Aug 4, 2021

Conversation

knmsk
Copy link
Contributor

@knmsk knmsk commented Jul 26, 2021

Added a way to ignore the scuttle inside starter and runner.

edit: Build off of #49, #53 and #54

Copy link
Contributor

@knechtionscoding knechtionscoding left a comment

Choose a reason for hiding this comment

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

Overall, I like this. Two general comments:

  1. Should we put the ENV variables for scuttle behind a bool as well? This would prevent unnecessary ENV or edge case interactions. But might be a big pain.

  2. Composition wise. Should we move the istio check into the helpers.go and let it return a command and env variables? So we can keep everything in the same place?

@knmsk
Copy link
Contributor Author

knmsk commented Jul 29, 2021

Should we put the ENV variables for scuttle behind a bool as well? This would prevent unnecessary ENV or edge case interactions. But might be a big pain.

Probably it's better to create a struct for this stuff.

Composition wise. Should we move the istio check into the helpers.go and let it return a command and env variables? So we can keep everything in the same place?

Sure, we can move into the helpers.go it makes sense!

@dgzlopes dgzlopes merged commit 5e1016f into grafana:main Aug 4, 2021
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.

3 participants