CLID-636: Update AGENTS.md and switch to .agents dir#1421
Conversation
WalkthroughDocumentation-only rewrite of AGENTS.md to mark v2 current, add explicit project and docs directory trees, update key architecture descriptions for collectors and image-copy concurrency, and expand build/test/validation guidance including testing patterns and build-tag examples. Changesoc-mirror Agent Guide Restructuring
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dorzel
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dorzel: This pull request references CLID-636 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 23: In AGENTS.md update the two fenced code blocks that currently use
plain ``` to include a language identifier (e.g., change ``` to ```text) so
markdownlint MD040 is satisfied; locate the two triple-backtick blocks that
contain the directory trees (the oc-mirror/ and docs/ code blocks) and prepend
"text" after the opening backticks (retain the existing block content and
closing backticks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cbf5fc82-0f74-41c5-b8b9-6d6e148580ad
📒 Files selected for processing (3)
.agents/commands/README.md.agents/commands/generate-imageset.mdAGENTS.md
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dorzel The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 65: Update the doc entry string in AGENTS.md to correct the typo: replace
"progress-bar-and-concurrencty-investigation.md" with
"progress-bar-and-concurrency-investigation.md" wherever it appears (the listed
entry under the progress/concurrency design bullet); ensure any internal links
or references that use this filename are updated to the corrected "concurrency"
spelling as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d08effa8-297a-40cb-9ae5-11a288d06d95
📒 Files selected for processing (3)
.agents/commands/README.md.agents/commands/generate-imageset.mdAGENTS.md
|
@dorzel: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| ## Project Overview | ||
|
|
||
| This is the `oc-mirror` repository - an OpenShift client tool for mirroring container registry content for disconnected cluster installs. | ||
| This is the `oc-mirror` repository — an OpenShift client tool for mirroring container registry content for disconnected cluster installs. |
There was a problem hiding this comment.
| This is the `oc-mirror` repository — an OpenShift client tool for mirroring container registry content for disconnected cluster installs. | |
| This is the `oc-mirror` repository - an OpenShift client tool (Command Line Interface - CLI) for mirroring container registry content for disconnected cluster installs. |
There was a problem hiding this comment.
Maybe just
| This is the `oc-mirror` repository — an OpenShift client tool for mirroring container registry content for disconnected cluster installs. | |
| This is the `oc-mirror` repository — an OpenShift command line tool for mirroring container registry content for disconnected cluster installs. |
There was a problem hiding this comment.
Adolfo suggestion is simpler, which is better.
| ├── pkg/ | ||
| │ ├── cli/mirror/ # top-level mirror orchestration (m2m, m2d, d2m) | ||
| │ └── metadata/storage/ # metadata/history persistence |
There was a problem hiding this comment.
In v2 there is no pkg/cli/mirror and pkg/metadata/storage
There was a problem hiding this comment.
I am not sure if it's worth to have the file trees here. This might change over time and it will become obsolete. I think agents can easily find out the repository structure dynamically. I think instead we should emphasize the high-level components and architecture (collector, batch worker, etc), their function and why they exist.
There was a problem hiding this comment.
I like the idea of explaining the high-level packages without having them in a tree structure, in this way when we refactor we don't need to remember to update the tree structure. Moving pieces from one place to another is a very common task and it would make the tree outdated easily.
| │ └── metadata/storage/ # metadata/history persistence | ||
| ├── internal/ | ||
| │ ├── pkg/ | ||
| │ │ ├── api/v2alpha1/ # ImageSetConfiguration types and schemas |
There was a problem hiding this comment.
There are other sub-folders under api, archive, cli, clusterresources, config, history. Also there are some pkgs missing in the list (spinners and version for example, there are others missing). Should we add all of them?
Each folder, even if it is a sub-folder it is considered a different pkg in golang, so maybe should we considere also sub-folders in the structure?
| │ │ └── ... # log, emoji, spinners, consts, etc. | ||
| │ ├── e2e/ # E2E test infrastructure and templates | ||
| │ └── testutils/ # Shared test helpers | ||
| ├── tests/ # Test fixtures (OCI images, configs, caches) |
There was a problem hiding this comment.
I believe there is a typo here.
| ├── tests/ # Test fixtures (OCI images, configs, caches) | |
| ├── tests/ # Test features (OCI images, configs, caches) |
|
|
||
| - **Unit vs integration**: `make test-unit` runs with `-short`. Integration tests check `testing.Short()` and skip themselves, so they only run via `make test-integration`. | ||
| - **Mocking**: tests use `github.com/stretchr/testify/mock`. Mock structs are defined alongside the tests that use them (see `internal/pkg/cli/executor_test.go` for examples). | ||
| - **Test fixtures**: stored in `tests/` and referenced via the constant `consts.TestFolder` (`"../../../tests/"`). Fixtures include fake OCI images, caches, configs, and archive data. |
There was a problem hiding this comment.
typo
| - **Test fixtures**: stored in `tests/` and referenced via the constant `consts.TestFolder` (`"../../../tests/"`). Fixtures include fake OCI images, caches, configs, and archive data. | |
| - **Test features**: stored in `tests/` and referenced via the constant `consts.TestFolder` (`"../../../tests/"`). Features include fake OCI images, caches, configs, and archive data. |
| ```bash | ||
| make verify # run golangci-lint | ||
| make sanity # runs: tidy, format, and vet checks | ||
| go test -tags "json1 exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp" \ |
There was a problem hiding this comment.
Not so sure if these tags are really needed. Maybe am I missing anything?
There was a problem hiding this comment.
If someone doesn't have all the deps installed, some of these flags might be needed.
| -short -race -count=1 ./internal/pkg/image/... | ||
|
|
||
| go test -tags "json1 exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp" \ | ||
| -short -race -count=1 -run TestSpecificName ./internal/pkg/release/... |
There was a problem hiding this comment.
@adolfo-ab is this the right way to call specific tests when using the new approach with ginko?
There was a problem hiding this comment.
It's pretty similar, yes
| ## Project Overview | ||
|
|
||
| This is the `oc-mirror` repository - an OpenShift client tool for mirroring container registry content for disconnected cluster installs. | ||
| This is the `oc-mirror` repository — an OpenShift client tool for mirroring container registry content for disconnected cluster installs. |
There was a problem hiding this comment.
Maybe just
| This is the `oc-mirror` repository — an OpenShift client tool for mirroring container registry content for disconnected cluster installs. | |
| This is the `oc-mirror` repository — an OpenShift command line tool for mirroring container registry content for disconnected cluster installs. |
| ├── pkg/ | ||
| │ ├── cli/mirror/ # top-level mirror orchestration (m2m, m2d, d2m) | ||
| │ └── metadata/storage/ # metadata/history persistence |
There was a problem hiding this comment.
I am not sure if it's worth to have the file trees here. This might change over time and it will become obsolete. I think agents can easily find out the repository structure dynamically. I think instead we should emphasize the high-level components and architecture (collector, batch worker, etc), their function and why they exist.
| ### Documentation tree | ||
|
|
||
| ```text | ||
| docs/ |
| -short -race -count=1 ./internal/pkg/image/... | ||
|
|
||
| go test -tags "json1 exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp" \ | ||
| -short -race -count=1 -run TestSpecificName ./internal/pkg/release/... |
There was a problem hiding this comment.
It's pretty similar, yes
|
|
||
| Run `make sanity` before committing — it will fail if there are uncommitted formatting or module changes. | ||
|
|
||
| ## Testing patterns |
There was a problem hiding this comment.
I think we can redirect the agent to docs/testing, which I just added here: #1422
5 participants
Description
Updates content in AGENTS.md and moves from .claude to .agents dir, expecting users to have a local symlink for use with specific agents.
Also fixes a typo.
Previous PR was accidentally closed after I renamed my branch: #1402.
Github / Jira issue: https://redhat.atlassian.net/browse/CLID-636
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit