Strip nonadmin oadp 1.4#212
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
do we even need admin detection?
There was a problem hiding this comment.
I do not believe so, will edit files to align with no admin detection
|
What a -10k nuke! |
Joeavaikath
left a comment
Joeavaikath
left a comment
There was a problem hiding this comment.
Don't forget to add the oadp-cli tests to the oadp-operator 1.4 version
|
|
||
| t.Run("all major commands exist", func(t *testing.T) { | ||
| majorCommands := []string{"backup", "restore", "nabsl-request", "nonadmin", "client", "version"} | ||
| majorCommands := []string{"backup", "restore", "schedule", "backup-location", "client", "version", "must-gather", "setup"} |
There was a problem hiding this comment.
Are these not in main/oadp-1.6/oadp-1.5?
There was a problem hiding this comment.
@coderabbitai Please check and if confirmed create issue track fix for other branches for missing schedule etc.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List branches
git branch -r | grep -E 'main|oadp-1\.[456]' || true
echo "---- Checking root command registrations per branch ----"
for br in origin/main origin/oadp-1.6 origin/oadp-1.5 origin/oadp-1.4; do
echo "== Branch: $br =="
git show "$br:integration_test.go" 2>/dev/null | grep -A3 'majorCommands :=' || echo "file/section not found"
echo
done
echo "---- Search for root command registration files ----"
for br in origin/main origin/oadp-1.6 origin/oadp-1.5 origin/oadp-1.4; do
echo "== Branch: $br =="
git ls-tree -r "$br" --name-only | grep -iE 'root\.go|cmd\.go' | head -20
doneLength of output: 5225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- remote branches ----"
git branch -r
echo "---- fetch all branches ----"
git fetch origin main oadp-1.6 oadp-1.5 oadp-1.4 2>&1 || true
for br in main oadp-1.6 oadp-1.5 oadp-1.4; do
echo "== Branch: $br =="
git show "origin/$br:integration_test.go" 2>/dev/null | grep -A3 'majorCommands :=' || echo "not found in integration_test.go"
echo "-- cmd root files --"
git ls-tree -r "origin/$br" --name-only 2>/dev/null | grep -iE '(^|/)(root|cmd)\.go$' | head -20
echo
doneLength of output: 1044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- List branches via gh api ----"
gh api repos/migtools/oadp-cli/branches --paginate --jq '.[].name'
echo "---- PR `#212` base branch ----"
gh pr view 212 --repo migtools/oadp-cli --json baseRefName,headRefNameLength of output: 785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for br in oadp-dev oadp-1.6 oadp-1.5 oadp-1.4; do
echo "======================== Branch: $br ========================"
echo "-- integration_test.go majorCommands --"
gh api "repos/migtools/oadp-cli/contents/integration_test.go?ref=$br" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | grep -A3 'majorCommands :=' || echo "not found"
echo
doneLength of output: 1601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for br in oadp-dev oadp-1.6 oadp-1.5 oadp-1.4; do
echo "======================== Branch: $br ========================"
echo "-- searching for AddCommand registrations of schedule/backup-location/must-gather/setup --"
gh api "repos/migtools/oadp-cli/git/trees/$br?recursive=true" --jq '.tree[].path' 2>/dev/null | grep -iE 'root\.go$|cmd\.go$'
doneLength of output: 1204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for br in oadp-dev oadp-1.6 oadp-1.5 oadp-1.4; do
echo "======================== Branch: $br ========================"
gh api "repos/migtools/oadp-cli/contents/cmd/root.go?ref=$br" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | grep -nE 'AddCommand|schedule|backup-location|must-gather|setup'
echo
doneLength of output: 4007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for br in oadp-dev oadp-1.6 oadp-1.5 oadp-1.4; do
echo "======================== Branch: $br ========================"
gh api "repos/migtools/oadp-cli/contents/cmd/root.go?ref=$br" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | grep -nE 'backup-location|backuplocation|NewBackupLocation|bsl'
echo
doneLength of output: 2721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
body=$(cat <<'EOF'
### Summary
`integration_test.go`'s `TestCommandArchitecture` test (`majorCommands` list in the "all major commands exist" subtest) only checks for `backup`, `restore`, `nabsl-request`, `nonadmin`, `client`, `version`. It does not assert the presence of `schedule`, `backup-location`, `must-gather`, and `setup`, even though `cmd/root.go` registers all of these commands via `schedule.NewCommand(f)`, `backuplocation.NewCommand(f)`, `mustgather.NewMustGatherCommand(f)`, and `setup.NewSetupCommand(f)`.
This was confirmed on `main`/`oadp-dev`, `oadp-1.6`, and `oadp-1.5` — the commands exist in `cmd/root.go` on all of them, but the integration test's command list was never updated to cover them.
PR `#212` fixes this only for the `oadp-1.4` branch (see https://github.com/migtools/oadp-cli/pull/212, comment https://github.com/migtools/oadp-cli/pull/212#discussion_r3506819793).
### Affected branches
- `main` / `oadp-dev`
- `oadp-1.6`
- `oadp-1.5`
### Required change
In `integration_test.go`, update the `majorCommands` slice in `TestCommandArchitecture`'s "all major commands exist" subtest to include `schedule`, `backup-location`, `must-gather`, and `setup` (mirroring the fix applied to `oadp-1.4` in PR `#212`).
### Acceptance criteria
- [ ] `main`/`oadp-dev` integration test asserts `schedule`, `backup-location`, `must-gather`, `setup` in root `--help` output
- [ ] `oadp-1.6` integration test updated similarly
- [ ] `oadp-1.5` integration test updated similarly
Requested by `@kaovilai`
EOF
)
gh issue create \
--repo migtools/oadp-cli \
--title "integration_test.go: TestCommandArchitecture missing assertions for schedule, backup-location, must-gather, setup on main/oadp-1.6/oadp-1.5" \
--body "$body" \
--assignee kaovilaiLength of output: 2184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
body=$(cat <<'EOF'
### Summary
`integration_test.go`'s `TestCommandArchitecture` test (`majorCommands` list in the "all major commands exist" subtest) only checks for `backup`, `restore`, `nabsl-request`, `nonadmin`, `client`, `version`. It does not assert the presence of `schedule`, `backup-location`, `must-gather`, and `setup`, even though `cmd/root.go` registers all of these commands via `schedule.NewCommand(f)`, `backuplocation.NewCommand(f)`, `mustgather.NewMustGatherCommand(f)`, and `setup.NewSetupCommand(f)`.
This was confirmed on `main`/`oadp-dev`, `oadp-1.6`, and `oadp-1.5` — the commands exist in `cmd/root.go` on all of them, but the integration test's command list was never updated to cover them.
PR `#212` fixes this only for the `oadp-1.4` branch (see https://github.com/migtools/oadp-cli/pull/212, comment https://github.com/migtools/oadp-cli/pull/212#discussion_r3506819793).
### Affected branches
- `main` / `oadp-dev`
- `oadp-1.6`
- `oadp-1.5`
### Required change
In `integration_test.go`, update the `majorCommands` slice in `TestCommandArchitecture`'s "all major commands exist" subtest to include `schedule`, `backup-location`, `must-gather`, and `setup` (mirroring the fix applied to `oadp-1.4` in PR `#212`).
### Acceptance criteria
- [ ] `main`/`oadp-dev` integration test asserts `schedule`, `backup-location`, `must-gather`, `setup` in root `--help` output
- [ ] `oadp-1.6` integration test updated similarly
- [ ] `oadp-1.5` integration test updated similarly
Requested by `@kaovilai`
EOF
)
gh issue create \
--repo migtools/oadp-cli \
--title "integration_test.go: TestCommandArchitecture missing assertions for schedule, backup-location, must-gather, setup on main/oadp-1.6/oadp-1.5" \
--body "$body"Length of output: 1962
@kaovilai Confirmed — checked cmd/root.go on oadp-dev/main, oadp-1.6, and oadp-1.5, and all of them already register schedule (schedule.NewCommand(f)), backup-location (backuplocation.NewCommand(f)), must-gather (mustgather.NewMustGatherCommand(f)), and setup (setup.NewSetupCommand(f)) as real commands. So the CLI functionality is present everywhere — it's integration_test.go's TestCommandArchitecture test that's out of date on those branches, since its majorCommands list only checks for backup, restore, nabsl-request, nonadmin, client, version and was never updated to cover the newer commands.
I opened a tracking issue: #215
Note: I wasn't able to assign it to you directly (the GitHub API returned a permissions error for assigning via the bot), so you may want to self-assign it manually.
(◕‿◕) all set — issue filed for the other branches
🧬 Code Graph Analysis Results
Relevant snippets for integration_test.go (user request about “missing schedule” across other branches)
internal/testutil/testutil.go
Lines 97-122 — RunCommand(t *testing.T, binaryPath string, args ...string) (string, error)
func RunCommand(t *testing.T, binaryPath string, args ...string) (string, error) {
t.Helper()
ctx, cancel := context.WithTimeout(context.Background(), TestTimeout)
defer cancel()
cmd := exec.CommandContext(ctx, binaryPath)
cmd.Args = append(cmd.Args, args...)
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
err := cmd.Run()
// Log the command and output for debugging
t.Logf("Command: %s %s", binaryPath, strings.Join(args, " "))
if stdout.Len() > 0 {
t.Logf("Stdout: %s", stdout.String())
}
if stderr.Len() > 0 {
t.Logf("Stderr: %s", stderr.String())
}
return stdout.String(), err
}Lines 40-62 — GetProjectRoot(t *testing.T) string
func GetProjectRoot(t *testing.T) string {
t.Helper()
// Get the directory of the current file
_, filename, _, ok := runtime.Caller(0)
if !ok {
t.Fatal("Failed to get caller information")
}
// Navigate up to find the project root (where go.mod is)
dir := filepath.Dir(filename)
for {
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
return dir
}
parent := filepath.Dir(dir)
if parent == dir {
t.Fatal("Could not find project root (go.mod not found)")
}
dir = parent
}
}Lines 65-94 — BuildCLIBinary(t *testing.T) string
func BuildCLIBinary(t *testing.T) string {
t.Helper()
projectRoot := GetProjectRoot(t)
// Create temp directory for the binary
tempDir := t.TempDir()
binaryName := "oadp-test"
if runtime.GOOS == "windows" {
binaryName += ".exe"
}
binaryPath := filepath.Join(tempDir, binaryName)
t.Logf("Building CLI binary: %s", binaryPath)
t.Logf("Project root: %s", projectRoot)
// Build the binary with a longer timeout for CI environments
ctx, cancel := context.WithTimeout(context.Background(), BuildTimeout)
defer cancel()
cmd := exec.CommandContext(ctx, "go", "build", "-o", binaryPath, ".")
cmd.Dir = projectRoot
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("Failed to build CLI binary: %v\nOutput: %s", err, string(output))
}
return binaryPath
}Lines 143-162 — SetupTempHome(t *testing.T) (string, func())
func SetupTempHome(t *testing.T) (string, func()) {
t.Helper()
tempHome := t.TempDir()
configDir := filepath.Join(tempHome, ".config", "velero")
if err := os.MkdirAll(configDir, 0755); err != nil {
t.Fatalf("Failed to create config directory: %v", err)
}
// Save original HOME
originalHome := os.Getenv("HOME")
os.Setenv("HOME", tempHome)
// Return cleanup function
cleanup := func() {
os.Setenv("HOME", originalHome)
}
return tempHome, cleanup
}cmd/setup/setup.go
Lines 51-83 — (*SetupOptions) Run(c *cobra.Command, f client.Factory) error
func (o *SetupOptions) Run(c *cobra.Command, f client.Factory) error {
c.SilenceUsage = true
// Check if already configured (unless --force flag set)
if !o.Force {
existingConfig, err := shared.ReadVeleroClientConfig()
if err != nil {
return fmt.Errorf("failed to read existing config: %w", err)
}
if existingConfig.Namespace != "" {
fmt.Println("OADP CLI is already configured.")
fmt.Println()
o.printCurrentConfig(existingConfig)
fmt.Println()
fmt.Println("To reconfigure, run: oc oadp setup --force")
return nil
}
}
config, err := shared.ReadVeleroClientConfig()
if err != nil {
return fmt.Errorf("failed to read existing config: %w", err)
}
if err := shared.WriteVeleroClientConfig(config); err != nil {
return fmt.Errorf("failed to write config: %w", err)
}
o.printSetupSuccess()
return nil
}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai, NicholasYancey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Why the changes were made
The OADP 1.4 operator does not ship the non-admin controller (RELATED_IMAGE_NON_ADMIN_CONTROLLER is absent from config/manager/manager.yaml on the oadp-1.4 branch, present on oadp-1.5). All non admin commands have been removed, along with the self service doc due to non admin not being supported in oadp-1.4. README and docs/OADP-1.4.md doc changes are handled separately in PR #210
How to test the changes made
make build./kubectl-oadp --helpshould show nononadminornabsl-requestsubcommandsoc oadp backup --help,oc oadp restore --help,oc oadp schedule --helpmake test