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.
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
continuous-test as module #7747
continuous-test as module #7747
Changes from 17 commits
6614cb5
fd184d4
4f20f22
fbea9ff
5dfcfe2
0a75513
222b5f0
fe999f9
4219c9d
8263e32
0f582e2
5fe76b1
50b16c3
654c6d5
574a045
095a137
56b41c2
30470f3
6c43b47
b4a650f
578d527
900c0d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Seems like we should add a YAML section if this is going to be a standard part of Mimir.
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.
I disagree as it doesn't run as a default startup piece of Mimir with target=all. It's currently usable as a standalone docker image with flags passed and doesn't take a config file and I think keeping it as close to current usage as possible is the best approach. I was including config for it originally and decided that passing config via file isn't something currently anyone has asked for so to hold off
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.
Other components of Mimir that have CLI flags have a corresponding YAML configuration. That makes this particular feature inconsistent.
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.
I agree re consistency. Adding YAML support will also expose all the continuous-test flags in reference documentation, which would be quite ugly, but now that continuous-test is part of Mimir, I think we should do it.
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.
Doesn't seem necessary to create aBetter idea belowstart
method here for the service. Should be fine to just execute this as regular code here and passnil
for the start methodThere 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.
Usually the way you'd use the
service
stuff is to havecontinoustest.Manager
implement theservice.Service
interface and return an instance of it from this method. Then the module system handles calling start and run methods.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.
True! That was the original design. I don't know how I got here from there, but happy to go back to it as that's how the other modules behave.
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.
Nope! Nevermind. I just converted it and got the errors I was getting beforehand when using the Manager as the service. Basically yes, that is the pattern we use in many other modules but they are meant to be continuously running with different behaviors for stopping etc. whereas if you look at ActivityTracker as an example for NewIdleService, it's worth it to use that dskit infrastructure instead of writing a new AwaitRunning func etc for it to behave properly. All I want is to init, and then be able to call Run and wrapping in BasicService does that here cleanly I think. I do agree with your previous comment for cleanly running it all as init, and then just pass it the Run func.