-
Notifications
You must be signed in to change notification settings - Fork 486
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!(exporter/blackbox): use required name attr instead label for target block #5945
feat!(exporter/blackbox): use required name attr instead label for target block #5945
Conversation
…rget block Signed-off-by: hainenber <[email protected]>
I'm on PTO so I shouldn't be on GitHub, but I saw the recommendation for a breaking change in #5940 and wanted to offer an alternative: Another way of doing this change that wouldn't be a breaking change is to have an optional attribute to override the target name, similar to the
|
Oh that's a good alternative right there. But I couldn't help but wonder if this is some kind of debt that can be cleared before v1.0.0 😄 If Erik's cool with this, I'm cool as well. |
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.
If we go ahead with a breaking change, it will need to be documented in docs/sources/flow/release-notes.md, which corresponds to Release notes for Grafana Agent Flow.
My personal preference would be to just do it as a breaking change, because this seems very minor. However, we should get some team consensus on this before proceeding with a specific approach.
Co-authored-by: Paulin Todev <[email protected]>
Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[email protected]>
Signed-off-by: hainenber <[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.
looks great, thanks for taking care of this!
I'll have a follow up PR likely to address some similar components we have identified.
My preference is to go with the breaking change before 1.0 instead of leaving a potential stumbling block behind. If someone uses a label they may waste time figuring out why |
I'm poking at the go test issue to see what is going on |
The latest one is A-OK now, the CI complained previously since I added tab instead of spaces :D |
A totally unrelated test had failed in the CI but I can't recreate it locally. I restarted the CI and hopefully it doesn't keep happening. |
…rget block (grafana#5945) * feat!(exporter/blackbox): use required name attr instead label for target block Signed-off-by: hainenber <[email protected]> --------- Signed-off-by: hainenber <[email protected]>
PR Description
Which issue(s) this PR fixes
Fixes #5940
Notes to the Reviewer
Should this be a breaking change? I gave it some thoughts and unless we specify an interface to allow interchangeability (which is awkward, IMO), breaking change it is since
label
block is mandatory atm.PR Checklist