Skip to content
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

analyse command #284

Closed
wants to merge 3 commits into from
Closed

analyse command #284

wants to merge 3 commits into from

Conversation

cragod
Copy link
Contributor

@cragod cragod commented Nov 24, 2023

Description

This is a draft of the analyse command which I used for debugging the ref issue, it is an extension based on warmup —analyse, which I believe is useful. And it could be extended to stages beyond hashing. For example:

rugby analyse hasher —missing-binary-only …
rugby analyse router …

I hope this suggestion sparks some ideas.

References

https://github.com/swiftyfinch/Rugby/issues/278

Checklist (I have ...)

  • 🧐 Followed the code style of the rest of the project
  • 📖 Updated the documentation, if necessary
  • 👨🏻‍🔧 Added at least one test which validates that my change is working, if appropriate
  • 👮🏻‍♂️ Run make lint and fixed all warnings
  • ✅ Run make test and fixed all tests

❤️ Thanks for contributing to the 🏈 Rugby!

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Attention: 56 lines in your changes are missing coverage. Please review.

Comparison is base (47410ea) 61.67% compared to head (c7cb4eb) 60.84%.

Files Patch % Lines
.../RugbyFoundation/Core/Analyse/AnalyseManager.swift 0.00% 56 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
- Coverage   61.67%   60.84%   -0.84%     
==========================================
  Files         140      141       +1     
  Lines        4063     4119      +56     
==========================================
  Hits         2506     2506              
- Misses       1557     1613      +56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@swiftyfinch
Copy link
Owner

swiftyfinch commented Dec 14, 2023

@cragod Hello! Is this pull request still actual?

  • I've finally finished with #236 😀
    Sorry, but you need to fix conflicts and make a rebase to main branch.
  • Also, I'm not sure about a separate Analyse command, because this naming means too much to me. But from another side it can be a call rugby analyse cache. If we need an another analyse subcommand, it will be reasonable to add it like this rugby analyse something. What do you think?
  • And it's not so great to duplicate warmup --analyse logic here. But I think that it's probably was my mistake to add it there. It looks better to me in the separate Analyse command. I need to think more about it)

@cragod
Copy link
Contributor Author

cragod commented Dec 14, 2023

@swiftyfinch hi, I still think this command is useful. We have already implemented the ability for developers to use CI cache through rugby. The analyse command can quickly help us identify the cause of cache misses.

The changes in this branch have been working well in our small-scale tests. After I'm done with my current work, I'll take some time to refactor it (including rebasing and implementing it as a subcommand).

Let me know if you have any new ideas.

@cragod cragod marked this pull request as ready for review December 24, 2023 09:58
@swiftyfinch
Copy link
Owner

Hello @cragod!
I've been thinking a lot about the idea and your way of implementing it.

I'm sorry to say that I'm not yet ready to incorporate it into the original 🏈 Rugby version at this time.
There isn't enough demand for this feature at the moment. I'm closing this pull request.

Right now, you can use the RUGBY_KEEP_HASH_YAMLS=YES rugby warmup --analyse command.
However, I understand that this isn't the same as your idea.

Also, I'd like to thank you for sharing your idea with me.
I appreciate all the input and suggestions that you have provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants