-
Notifications
You must be signed in to change notification settings - Fork 7
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
Migrate metrics agent layer to Struct API and bullet Stream #321
Conversation
Also update integration test to verify build output
As noted in the removed comment: It doesn't seem necessary as the layer env isn't explicitly modified as far as I can tell -- but perhaps there was some trait API magic happening I'm not familiar with?
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 looked at all the commits. They're easy to follow in isolation. You were right. It was quite easy to review.
I left one comment about why I left some code in that seemingly did nothing, it was intentional. Basically I'm being defensive about guarding against possible breaking future modifications. I'm curious if that description makes sense and your thoughts on the relative costs/benefits.
To be honest, I should probably not have included e0733ed on this branch. I wrote each commit as examples of focused types of changes that could be reviewed/integrated/merged better and faster independently (as separate PRs), and ended up confusing that by opening a PR including all the changes. To walk through my thought process (and noting things I could've done better in hindsight/based on your review):
I've talked a lot about PRs here, but hope it's also clear what I really care about: Clean, focused, atomic commits, that are able to stand on its own (e.g. be reviewed and tested independently, without introducing regressions) and make just one type of change (and never more than one logical change). GitHub PRs can be a great tool in that effort, not least because they trigger CI workflows that can help during development. But as far as I'm concerned, if the commits make sense on their own, you can basically structure them however you like in PRs. At any rate: Sounds like you want to iterate based on this branch -- I've approved the two PRs building on it, but haven't merged anything. Feel free to proceed however you prefer :) |
* Use metadata as single point of truth (SPOT) Previously we were using a metadata structure that was decoupled from the work being done (stored in the DOWNLOAD_URL constant). This created the possibility to update one without the other. * Style URLs * [Stacked] Fixes CI linting (#323) * Clippy recommended method * Allow deprecated Layer use * Clippy lint suggestions * Allow deprecated layer API usage * Allow deprecated layer API * Fewer allow(deprecation)-s
Migrate metrics agent layer to Struct API and bullet Stream. Taking over from #320 with a smaller and more modular process.