[receiver/purefa] Add validate on config components #17449
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Foresight Summary
View More Details✅ tracegen workflow has finished in 56 seconds (1 minute 31 seconds less than
|
| Job | Failed Steps | Tests | |
|---|---|---|---|
| build-dev | - 🔗 | N/A | See Details |
| publish-latest | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
⭕ build-and-test-windows workflow has finished in 5 seconds (33 minutes 26 seconds less than main branch avg.) and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| windows-unittest-matrix | - 🔗 | N/A | See Details |
| windows-unittest | - 🔗 | N/A | See Details |
✅ check-links workflow has finished in 43 seconds and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changed files | - 🔗 | N/A | See Details |
| check-links | - 🔗 | N/A | See Details |
✅ telemetrygen workflow has finished in 1 minute 6 seconds and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| publish-latest | - 🔗 | N/A | See Details |
| build-dev | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
✅ build-and-test workflow has finished in 35 minutes 52 seconds (13 minutes 42 seconds less than main branch avg.) and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| unittest-matrix (1.20, connector) | - 🔗 | ✅ 126 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, connector) | - 🔗 | ✅ 126 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-metrics | - 🔗 | ✅ 2 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, internal) | - 🔗 | ✅ 583 ❌ 0 ⏭ 0 🔗 | See Details |
| correctness-traces | - 🔗 | ✅ 17 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, extension) | - 🔗 | ✅ 544 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, processor) | - 🔗 | ✅ 1557 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, extension) | - 🔗 | ✅ 544 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, internal) | - 🔗 | ✅ 583 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, processor) | - 🔗 | ✅ 1557 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, receiver-0) | - 🔗 | ✅ 2614 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-0) | - 🔗 | ✅ 2614 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, exporter) | - 🔗 | ✅ 2506 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, exporter) | - 🔗 | ✅ 2506 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, receiver-1) | - 🔗 | ✅ 1965 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, other) | - 🔗 | ✅ 4756 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.20, receiver-1) | - 🔗 | ✅ 1965 ❌ 0 ⏭ 0 🔗 | See Details |
| integration-tests | - 🔗 | ✅ 56 ❌ 0 ⏭ 0 🔗 | See Details |
| unittest-matrix (1.19, other) | - 🔗 | ✅ 4756 ❌ 0 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
| check-collector-module-version | - 🔗 | N/A | See Details |
| build-examples | - 🔗 | N/A | See Details |
| check-codeowners | - 🔗 | N/A | See Details |
| checks | - 🔗 | N/A | See Details |
| lint-matrix (receiver-0) | - 🔗 | N/A | See Details |
| lint-matrix (receiver-1) | - 🔗 | N/A | See Details |
| lint-matrix (processor) | - 🔗 | N/A | See Details |
| lint-matrix (exporter) | - 🔗 | N/A | See Details |
| lint-matrix (extension) | - 🔗 | N/A | See Details |
| lint-matrix (connector) | - 🔗 | N/A | See Details |
| lint-matrix (internal) | - 🔗 | N/A | See Details |
| lint-matrix (other) | - 🔗 | N/A | See Details |
| lint | - 🔗 | N/A | See Details |
| unittest (1.19) | - 🔗 | N/A | See Details |
| unittest (1.20) | - 🔗 | N/A | See Details |
| cross-compile (darwin, amd64) | - 🔗 | N/A | See Details |
| cross-compile (darwin, arm64) | - 🔗 | N/A | See Details |
| cross-compile (linux, amd64) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm) | - 🔗 | N/A | See Details |
| cross-compile (linux, arm64) | - 🔗 | N/A | See Details |
| cross-compile (linux, ppc64le) | - 🔗 | N/A | See Details |
| cross-compile (windows, 386) | - 🔗 | N/A | See Details |
| cross-compile (windows, amd64) | - 🔗 | N/A | See Details |
| cross-compile (linux, 386) | - 🔗 | N/A | See Details |
| build-package (deb) | - 🔗 | N/A | See Details |
| build-package (rpm) | - 🔗 | N/A | See Details |
| windows-msi | - 🔗 | N/A | See Details |
| publish-check | - 🔗 | N/A | See Details |
| publish-dev | - 🔗 | N/A | See Details |
| publish-stable | - 🔗 | N/A | See Details |
✅ changelog workflow has finished in 2 minutes 41 seconds and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| changelog | - 🔗 | N/A | See Details |
✅ load-tests workflow has finished in 6 minutes 58 seconds (3 minutes 52 seconds less than main branch avg.) and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| loadtest (TestTraceAttributesProcessor) | - 🔗 | ✅ 3 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestIdleMode) | - 🔗 | ✅ 1 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) | - 🔗 | ✅ 8 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetric10kDPS|TestMetricsFromFile) | - 🔗 | ✅ 6 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) | - 🔗 | ✅ 10 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestMetricResourceProcessor|TestTrace10kSPS) | - 🔗 | ✅ 12 ❌ 0 ⏭ 0 🔗 | See Details |
| loadtest (TestBallastMemory|TestLog10kDPS) | - 🔗 | ✅ 18 ❌ 0 ⏭ 0 🔗 | See Details |
| setup-environment | - 🔗 | N/A | See Details |
✅ prometheus-compliance-tests workflow has finished in 10 minutes 23 seconds (⚠️ 4 minutes 5 seconds more than main branch avg.) and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| prometheus-compliance-tests | - 🔗 | ✅ 21 ❌ 0 ⏭ 0 🔗 | See Details |
✅ e2e-tests workflow has finished in 11 minutes 58 seconds (2 minutes 50 seconds less than main branch avg.) and finished at 28th Mar, 2023.
| Job | Failed Steps | Tests | |
|---|---|---|---|
| kubernetes-test (v1.26.0) | - 🔗 | N/A | See Details |
| kubernetes-test (v1.24.7) | - 🔗 | N/A | See Details |
| kubernetes-test (v1.23.13) | - 🔗 | N/A | See Details |
| kubernetes-test (v1.25.3) | - 🔗 | N/A | See Details |
*You can configure Foresight comments in your organization settings page.
|
You might want to add a unit test covering validation checks. |
bogdandrutu
reviewed
Jan 21, 2023
bogdandrutu
approved these changes
Jan 21, 2023
bogdandrutu
previously requested changes
Jan 24, 2023
jpkrohling
previously approved these changes
Jan 25, 2023
|
Looks like the go.mod/sum are outdated now. |
jpkrohling
approved these changes
Jan 25, 2023
jpkrohling
reviewed
Jan 25, 2023
receiver/purefareceiver/config.go
Outdated
| var err error | ||
|
|
||
| if c.Settings.ReloadIntervals.Array.String() == "" { | ||
| err = multierr.Append(err, errors.New("Arrays not provided and is required")) |
There was a problem hiding this comment.
Golang convention is that errors messages being in lower case
receiver/purefareceiver/config.go
Outdated
| @@ -64,6 +66,23 @@ type ReloadIntervals struct { | |||
| } | |||
|
|
|||
| func (c *Config) Validate() error { | |||
| // TODO(dgoscn): perform config validation | |||
| return nil | |||
| var err error | |||
There was a problem hiding this comment.
Minor nit: name this as errs since this represents all errors found in the config validation.
|
Hey @dgoscn, Are you still working on this change? |
Hi @MovieStoreGuy. Thanks for your review. Yes, I am working on this. I will push de changes asap. Thank you |
MovieStoreGuy
added
ready to merge
jpkrohling
force-pushed
the
purestorage_flasharray_receiver_config_validate
branch
from
e7555c0
to
1e9d113
Compare
|
@bogdandrutu, I believe @dgoscn is addressing @MovieStoreGuy's concerns now, but I see you are still blocking this PR. Could you please take a look and see if there are blockers left here? |
jpkrohling
requested changes
Feb 8, 2023
jpkrohling
approved these changes
Feb 8, 2023
|
Hey @dgoscn , Could I ask you to rebase with main please? |
Sure @MovieStoreGuy thansk for the review |
|
The go.mod/sum files are still outdated. |
dgoscn
force-pushed
the
purestorage_flasharray_receiver_config_validate
branch
2 times, most recently
from
9ea37e3
to
7ce3323
Compare
MovieStoreGuy
approved these changes
Mar 19, 2023
|
Please rebase. |
|
Once this is rebased, I'll review and merge this. |
Signed-off-by: dgoscn <diego-ii@live.com>
This reverts commit d4ab00772c99284993e4153d8ce1d143efdab35b.
dgoscn
force-pushed
the
purestorage_flasharray_receiver_config_validate
branch
from
f5c6aaa
to
1798bb4
Compare
|
Hi, @jpkrohling. Thank you for your last comment. Can you please check my previous commit? Thank you |
jpkrohling
approved these changes
Mar 29, 2023
jpkrohling
dismissed
bogdandrutu’s stale review
sorry to dismiss your review, but you don't seem to have responded to previous requests for re-reviews -- if you feel like this should still be done differently, let me know and I'll work with @dgoscn on getting that ammended
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
There were missing code snippet from validate section on config.go inside the purefa receiver. At this piece of code, a block is inserted and the following unit test.
Link to tracking Issue:
#14886
Testing:
Same present on README.md section
Documentation:
Still in progress