-
Notifications
You must be signed in to change notification settings - Fork 183
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
Create fields automatically in setup job #1064
Conversation
d46d2e8
to
db2fac5
Compare
db2fac5
to
7efe094
Compare
fd5b742
to
f8e21e4
Compare
f8e21e4
to
75579c5
Compare
8a33aac
to
d7df24d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but left a few comments
We've tried to automatically create fields. In an unlikely scenario that this | ||
fails please refer to the following to create them manually: | ||
https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/2b3ca63/deploy/docs/Installation_with_Helm.md#prerequisite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no easy way to only show the text in the event of an actual failure I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was discussed and we couldn't figure out a way of getting return feedback from the setup job to helm to conditionally render this message.
variable "create_fields" { | ||
description = "If set, terraform will attempt to create fields at Sumo Logic" | ||
type = bool | ||
default = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why default to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason. I've provided the default as suggested by @sumo-drosiek prevent the need for specifying terraform variable during imports.
We can also default to true
if that's more appropriate for our use case. It will be specified on terraform apply
anyway
# Apply planned changes | |
terraform apply -auto-approve \ | |
-var="create_fields=${CREATE_FIELDS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with folks on PL sync I've changed this to true
(which doesn't really change anything in our code but expresses our intent of creating fields by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit around function name and the result it produces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
Use https://github.com/SumoLogic/terraform-provider-sumologic/ (version
v2.3.0
introduced fields support) to create fields in setup job.Testing performed