feat(cli): make quickstart catalog repository configurable via --catalog-repo flag#6364
feat(cli): make quickstart catalog repository configurable via --catalog-repo flag#6364sachin9058 wants to merge 15 commits into
Conversation
The way to do this is to Thinking about the amount of code and a desire to test it, we should probably put the clone and find directories code in |
Got it, that makes sense thanks! I’ve moved away from depending on #6357 and implemented the clone logic inline for now so the PR is self-contained. That said, I agree it would be better to structure this under Let me update the PR with that approach 👍 |
…quickstart from clone helper
evankanderson
left a comment
There was a problem hiding this comment.
Rather than just converting the existing rule loading, we should probably do something more extensive where we load a profile, ruletypes, and supporting datasources from one of the README enabled directories in minder-rules-and-profiles?
That way, we can include the description of what the profile will do in the guided setup.
|
@evankanderson That makes sense, I like that direction 👍 Right now I focused on keeping the existing quickstart behavior but sourcing the rule type and profile from the repo. Extending this to support README-based directories with profiles, rule types, and datasources would definitely make the flow more flexible and user-friendly. I can open a follow-up PR to explore that approach once this one is in, if that sounds good. |
evankanderson
left a comment
There was a problem hiding this comment.
I think there's a bit more refactoring we could do to lean towards loading files from a filesystem (if we package it right, we could even use https://pkg.go.dev/embed#hdr-File_Systems to supply a backup set of rules that don't need to clone).
… for quickstart Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
a25afdc to
b1766e1
Compare
|
@evankanderson Thanks for the detailed feedback — I’ve reworked the implementation accordingly.
I also resolved the merge conflicts and cleaned up the commit history to keep this PR focused on the quickstart changes. Please let me know if you’d like me to refine anything further or adjust the approach. |
evankanderson
left a comment
There was a problem hiding this comment.
I'm not sure exactly what behavior we're expecting here -- it seems like the current code will try to load all the generic ruletypes and profiles from minder-rules-and-profiles into one project, which seems like it might be overload for a new user.
Additionally, if this work is laying the foundation for future refinement, I'd expect it to be adjusting / improving existing code (e.g. leveraging the same abstractions as the apply and create CLI commands), rather than implementing a lot of heavy new logic in cmd/. (Quickstart already has a bunch of heavy prompting logic, but it would be great to reduce the amount of logic in cmd/ (which doesn't contribute to coverage numbers), and increase the logic in internal/util/cli and pkg/.
|
@evankanderson Thanks for the detailed feedback — I’ve made a pass to align the implementation accordingly.
I’ve kept the current loading behavior as-is for now and can iterate on making it more selective in a follow-up PR if that direction makes sense. Let me know if there’s anything else you’d like refined. |
evankanderson
left a comment
There was a problem hiding this comment.
This is looking better, but I think with a few updates to fileconvert, this could be even simpler.
|
@evankanderson Thanks I’ve updated the implementation to better align with the existing abstractions.
Let me know if you’d prefer further alignment with existing helpers like |
evankanderson
left a comment
There was a problem hiding this comment.
Some additional suggested changes -- the overall goal is to reduce the amount of code that needs to be maintained / fixed in the future while increasing its capabilities -- right now this is about +250 lines of non-test library code, but I think we can get it closer to +100 while supporting adding repository load for additional sub-commands in a subsequent PR.
…oder
Restore absolute path normalization with osfs.New("/") after testing showed regression
with osfs.New("."). This ensures existing fileconvert tests and relative path handling
continue to work as expected.
Also includes prior refactors to unify filesystem expansion and remove duplication.
|
@evankandersonThanks again for the detailed feedback this was very helpful in shaping the final direction. I’ve made the following updates:
Let me know if this looks aligned now or if you’d like any further adjustments. |
…e into main PR - Converted tests to table-driven format - Added behavior-based assertions for profile filtering - Covered empty, valid, and mixed catalog scenarios - Aligned with project testing patterns
|
I’ve folded the test coverage into this PR so implementation and tests can be reviewed together. I also refactored the tests into a table-driven format to align with existing patterns and added behavior-focused assertions for profile validation. This should also make it easier to evaluate coverage impact as part of this PR. Let me know if this looks good or if you’d like any further refinements. |
|
Hi @evankanderson can u take a look on this again i have made the changes that aligns with your feedback tell me if it looks good or need more refinements or improvements ?? |
Summary
Add a
--catalog-repoflag to thequickstartcommand to allow users to specify a custom repository for loading rule and profile catalogs.Previously, the catalog repository URL was hardcoded, limiting flexibility to the default Minder catalog. This change enables users to provide their own repository while preserving the existing default behavior.
If the flag is not provided, the quickstart flow continues to use the default Minder catalog repository.
Additionally, the help text has been improved to better describe the purpose and usage of the flag.
Testing
make lint-fix go test ./...go run ./cmd/cli quickstart --catalog-repo