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

fix benthos.yaml config logic bug #29

Closed
wants to merge 7 commits into from

Conversation

sytgj7896321
Copy link

@sytgj7896321 sytgj7896321 commented Nov 3, 2022

no matter benthos run on streams mode or not, benthos.yaml is always needed, because some configuration can only be written in this file, please leave this file controlled by user itself

Yi-tao Shi added 2 commits November 3, 2022 14:43
…needed, because some configuration can only be written in this file, please leave this file controlled by user itself
templates/configmap.yaml Outdated Show resolved Hide resolved
Copy link

@wrdls wrdls left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not a maintainer.

@difabion
Copy link
Collaborator

Hello,
Please check the env support implemented in 0.7.0 and see if that suits your needs. There is definitely a bug in the config logic; having streams set to false shouldn't be disabling the rest of the block. Let me mess with it this week and get it fixed, bear with me!

@sytgj7896321 sytgj7896321 changed the title fix empty benthos.yaml bug and add environment variables support fix benthos.yaml config logic bug Feb 6, 2023
Yi-tao Shi added 2 commits February 6, 2023 10:57
@sytgj7896321 sytgj7896321 reopened this Feb 6, 2023
@sytgj7896321
Copy link
Author

sytgj7896321 commented Feb 6, 2023

Hello
Now we have env support in version 0.7.2, but config logic bug still not fixed.

In stream mode, benthos.yaml still always be like this:

http:
  enabled: true
  address: 0.0.0.0:4195
  root_path: /benthos
  debug_endpoints: false

But it should be like this, because metrics, logger and some other fields must be in benthos.yaml:

http:
  enabled: true
  address: 0.0.0.0:4195
  root_path: /benthos
  debug_endpoints: false
metircs:
  mapping: |
    meta = deleted()
  prometheus:
    push_url: "prometheus-pushgateway.monitoring:9091"
    push_job_name: benthos_push
    push_interval: 5s
    add_process_metrics: true
    add_go_metrics: true
logger:
  level: all
  format: json
  add_timestamp: true

@difabion

@difabion
Copy link
Collaborator

Thanks for the update - this makes sense and I don't remember if I considered the global configs in streams mode, sorry about that. I would like to retain the config block at the end to inform a user that benthos is running essentially config-less. Let me take a crack at a happy middle ground and propose it here.

@maxsargentdev
Copy link

bump

@charlie-haley
Copy link
Collaborator

Closing as completed. See #44 (comment) for more details.

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

5 participants