-
Notifications
You must be signed in to change notification settings - Fork 231
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
support multiple yaml config files #218
Conversation
Codecov ReportBase: 99.64% // Head: 99.64% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 71 72 +1
Lines 5061 5083 +22
=======================================
+ Hits 5043 5065 +22
Misses 12 12
Partials 6 6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I am not sure this merge function would work well. because the array is nested in the notify section. and for settings, we should merge the settings instead of overwriting the whole settings I think we can do this better. how about just using the yq? here is an example use it as a library: https://github.com/kamolhasan/yq-for-strings |
The settings are merged already, the example maybe a little misleading, so I modified it and will add more tests later. The merge code is simple now, I will research the yq to see if there is a better solution. |
At least, I think we must pass the following mandatory test cases: 1. Section Merge Caseintohttp:
- name: "test 1"
url: "http://localhost:8080"
method: "GET"
tcp:
- name: "test 2"
host: "localhost:8080" fromnotify:
slack:
- name: "slack"
webhook: "https://hooks.slack.com/services/xxxxxx" resulthttp:
- name: "test 1"
url: "http://localhost:8080"
method: "GET"
tcp:
- name: "test 2"
host: "localhost:8080"
notify:
slack:
- name: "slack"
webhook: "https://hooks.slack.com/services/xxxxxx" 2. Same Probe Merge Caseintohttp:
- name: "test 1"
url: "http://localhost:8080"
method: "GET" fromhttp:
- name: "test 2"
url: "http://localhost:8181"
method: "GET" resulthttp:
- name: "test 1"
url: "http://localhost:8080"
method: "GET"
- name: "test 2"
url: "http://localhost:8181"
method: "GET" 3. Notify Merge Caseintonotify:
slack:
- name: slack
webhook: "https://hooks.slack.com/services/xxxxxx" fromnotify:
discord:
- name: discord
webhook: "https://discord.com/api/webhooks/xxxxxx" resultnotify:
slack:
- name: slack
webhook: "https://hooks.slack.com/services/xxxxxx"
discord:
- name: discord
webhook: "https://discord.com/api/webhooks/xxxxxx" 4. Notify Array Merge Caseintonotify:
slack:
- name: slack1
webhook: "https://hooks.slack.com/services/xxxxxx" fromnotify:
slack:
- name: slack2
webhook: "https://hooks.slack.com/services/xxxxxx" resultnotify:
slack:
- name: slack1
webhook: "https://hooks.slack.com/services/xxxxxx"
- name: slack2
webhook: "https://hooks.slack.com/services/xxxxxx" 5. Settings Merge Caseintosettings:
name: easeprobe
sla:
schedule: "daily"
time: "00:00"
notify:
retry:
times: 5
interval: 10 fromsettings:
name: easeprobe_from
probe:
timeout: 10s
interval: 30s
sla:
schedule: "weekly" resultsettings:
name: easeprobe_from
probe:
timeout: 10s
interval: 30s
notify:
retry:
times: 5
interval: 10
sla:
schedule: "weekly" |
Co-authored-by: Hao Chen <[email protected]>
Co-authored-by: Hao Chen <[email protected]>
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
This PR is trying to resolve #201 by merging multiple yaml files.
The merge work is completed by https://github.com/mikefarah/yq using merge operator
*
and append arrays flag+
, more details can refer https://mikefarah.gitbook.io/yq/operators/multiply-merge.