Skip to content

feat(cli): make quickstart catalog repository configurable via --catalog-repo flag#6364

Open
sachin9058 wants to merge 15 commits into
mindersec:mainfrom
sachin9058:feat/quickstart-configurable-repo
Open

feat(cli): make quickstart catalog repository configurable via --catalog-repo flag#6364
sachin9058 wants to merge 15 commits into
mindersec:mainfrom
sachin9058:feat/quickstart-configurable-repo

Conversation

@sachin9058
Copy link
Copy Markdown
Contributor

Summary

Add a --catalog-repo flag to the quickstart command 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

  • Ran:
    make lint-fix
    go test ./...
    
  • Verified CLI flag parsing by running:
    go run ./cmd/cli quickstart --catalog-repo
  • Confirmed default behavior remains unchanged when no flag is provided
  • Confirmed custom repository value is correctly read and passed into the catalog loading flow
  • Verified fallback logic remains intact when repository loading fails

@sachin9058 sachin9058 requested a review from a team as a code owner April 14, 2026 12:55
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 14, 2026

Coverage Status

Coverage is 60.268%sachin9058:feat/quickstart-configurable-repo into mindersec:main. No base build found for mindersec:main.

@evankanderson
Copy link
Copy Markdown
Member

@evankanderson This PR depends on the in-memory repository clone utility introduced in #6357.

The quickstart changes use the internal/repo package from that PR. Once it is merged, CI/build issues related to missing imports will be resolved.

The way to do this is to git rebase this branch onto that one.

Thinking about the amount of code and a desire to test it, we should probably put the clone and find directories code in internal/cli.

@sachin9058
Copy link
Copy Markdown
Contributor Author

sachin9058 commented Apr 14, 2026

@evankanderson

@evankanderson This PR depends on the in-memory repository clone utility introduced in #6357.
The quickstart changes use the internal/repo package from that PR. Once it is merged, CI/build issues related to missing imports will be resolved.

The way to do this is to git rebase this branch onto that one.

Thinking about the amount of code and a desire to test it, we should probably put the clone and find directories code in internal/cli.

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 internal/cli for clarity and testability. I’ll refactor the current implementation accordingly.

Let me update the PR with that approach 👍

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sachin9058
Copy link
Copy Markdown
Contributor Author

@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.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go
… for quickstart

Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
@sachin9058 sachin9058 force-pushed the feat/quickstart-configurable-repo branch 2 times, most recently from a25afdc to b1766e1 Compare April 22, 2026 21:50
@sachin9058
Copy link
Copy Markdown
Contributor Author

@evankanderson Thanks for the detailed feedback — I’ve reworked the implementation accordingly.

  • Switched to filesystem-driven loading for rule types and profiles from the catalog repository
  • Added validation to ensure profiles only reference existing rule types
  • Removed the previous hardcoded/single-file approach
  • Structured the flow as load → validate → prompt → apply
  • Made the apply step transactional with rollback on failure

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.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/.

Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
Comment thread cmd/cli/app/quickstart/quickstart.go Outdated
@sachin9058
Copy link
Copy Markdown
Contributor Author

@evankanderson Thanks for the detailed feedback — I’ve made a pass to align the implementation accordingly.

  • Moved catalog loading and validation into internal/cli, keeping cmd/quickstart focused on orchestration
  • Reused existing utilities (profiles.ReadProfileFromPath, fileconvert, billyutil.Walk) instead of custom logic
  • Simplified data structures and loops, and cleaned up error handling
  • Updated behavior to skip invalid resources with warnings rather than failing the entire load
  • Removed unnecessary checks and made resource handling consistent

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.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking better, but I think with a few updates to fileconvert, this could be even simpler.

Comment thread pkg/profiles/util.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread pkg/profiles/util.go Outdated
@sachin9058
Copy link
Copy Markdown
Contributor Author

@evankanderson Thanks I’ve updated the implementation to better align with the existing abstractions.

  • Replaced custom file traversal and parsing with a reusable ResourcesFromFilesystem helper in fileconvert
  • Reduced duplication in catalog loading by reusing that helper for both rule types and profiles
  • Moved validation logic into a method on Catalog
  • Kept CLI logic focused on orchestration

Let me know if you’d prefer further alignment with existing helpers like ResourcesFromPaths, or if this direction looks good.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/fileconvert/encodedecode.go Outdated
Comment thread pkg/fileconvert/encodedecode.go Outdated
Comment thread pkg/fileconvert/fileconvert_test.go Outdated
Comment thread pkg/fileconvert/fileconvert_test.go
Comment thread pkg/fileconvert/collect.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread internal/cli/catalog.go Outdated
Comment thread cmd/cli/app/profile/common.go
Comment thread pkg/profiles/util.go Outdated
…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.
@sachin9058
Copy link
Copy Markdown
Contributor Author

@evankandersonThanks again for the detailed feedback this was very helpful in shaping the final direction.

I’ve made the following updates:

  • Moved filesystem expansion fully into util.ExpandFileArgs and removed duplicate traversal logic
  • Simplified collect.go to rely on the unified expansion flow
  • Removed the unused ReadProfileFromPath helper and related import
  • Fixed decoder path handling to preserve previous behavior for OS-backed paths
  • Verified all affected packages with tests and lint (go test and make lint-fix both pass)

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
@sachin9058
Copy link
Copy Markdown
Contributor Author

@evankanderson

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.

@sachin9058
Copy link
Copy Markdown
Contributor Author

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 ??

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.

3 participants