Skip to content

Conversation

@Joeavaikath
Copy link
Contributor

@Joeavaikath Joeavaikath commented Jun 27, 2025

  • Restructure non-admin dir
  • Add tests
  • Add github actions for tests and linting

@Joeavaikath Joeavaikath linked an issue Jun 27, 2025 that may be closed by this pull request
@Joeavaikath Joeavaikath removed a link to an issue Jun 27, 2025
@Joeavaikath Joeavaikath linked an issue Jun 27, 2025 that may be closed by this pull request
@Joeavaikath Joeavaikath moved this to In Progress in oadp-cli v0.1 Jun 27, 2025
@Joeavaikath Joeavaikath moved this to In Progress in oadp-cli v0.1 Jun 27, 2025
@kaovilai kaovilai requested a review from Copilot June 27, 2025 15:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR restructures the non-admin directory and adds comprehensive integration tests for the CLI. Key changes include updates to module and import paths, addition of new test files and utilities, and reorganization of non-admin backup commands.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

File Description
tests/* New test files for CLI commands, build processes, and documentation
main.go, go.mod Updated module and import paths to match the new repository structure
cmd/* Updated non-admin command implementations and restructured backup commands
.github/workflows/* Added CI configurations for tests and linting
Comments suppressed due to low confidence (1)

cmd/non-admin/backup/nonadminbackup_builder.go:17

  • The file name 'nonadminbackup_builder.go' is inconsistent with the package name 'backup'. Consider renaming the file to 'backup_builder.go' for clarity and consistency.
package backup

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

Thank you for adding comprehensive testing infrastructure to the project! The test structure is well-organized and the CI/CD workflows are a great addition. However, there are a few formatting and configuration issues that need to be addressed before merging.

Issues to Fix:

1. README.md - Incomplete code block (Line 105)

The code block is missing closing backticks:

-go test ./...
+go test ./...
+```

### 2. **Missing newlines at end of files**
Please add newlines at the end of these files:
- `.github/workflows/lint.yml`
- `.github/workflows/test.yml`
- `tests/README.md`

### 3. **Go version mismatch**
The `go.mod` file specifies `go 1.24.3` which doesn't exist. This should match the CI workflows which use Go 1.21:
```diff
-go 1.24.3
+go 1.21

Pick one or the other. I would match go version to ones used in the targeting velero version.

Positive Aspects:

  • ✅ Excellent test documentation in tests/README.md
  • ✅ Well-structured test suite with clear separation of concerns
  • ✅ Good CI/CD implementation with comprehensive linting
  • ✅ Clean refactoring of the non-admin directory structure

Once these formatting issues are fixed, this will be a great addition to the project!

Copy link
Contributor

@NicholasYancey NicholasYancey left a comment

Choose a reason for hiding this comment

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

Looks good, the unit test will be helpful in the near future!

@Joeavaikath
Copy link
Contributor Author

Thank you for adding comprehensive testing infrastructure to the project! The test structure is well-organized and the CI/CD workflows are a great addition. However, there are a few formatting and configuration issues that need to be addressed before merging.

Issues to Fix:

1. README.md - Incomplete code block (Line 105)

The code block is missing closing backticks:

-go test ./...
+go test ./...
+```

### 2. **Missing newlines at end of files**
Please add newlines at the end of these files:
- `.github/workflows/lint.yml`
- `.github/workflows/test.yml`
- `tests/README.md`

### 3. **Go version mismatch**
The `go.mod` file specifies `go 1.24.3` which doesn't exist. This should match the CI workflows which use Go 1.21:
```diff
-go 1.24.3
+go 1.21

Pick one or the other. I would match go version to ones used in the targeting velero version.

Positive Aspects:

  • ✅ Excellent test documentation in tests/README.md
  • ✅ Well-structured test suite with clear separation of concerns
  • ✅ Good CI/CD implementation with comprehensive linting
  • ✅ Clean refactoring of the non-admin directory structure

Once these formatting issues are fixed, this will be a great addition to the project!

k8s.io/client-go v0.33.1 → requires go 1.24.0
k8s.io/apimachinery v0.33.1 → requires go 1.24.0

actions to 1.24

@Joeavaikath Joeavaikath requested a review from kaovilai June 27, 2025 17:02
@Joeavaikath Joeavaikath merged commit 63b63ef into main Jun 27, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in oadp-cli v0.1 Jun 27, 2025
@Joeavaikath Joeavaikath linked an issue Jul 3, 2025 that may be closed by this pull request
This was unlinked from issues Jul 3, 2025
@Joeavaikath Joeavaikath linked an issue Jul 3, 2025 that may be closed by this pull request
@Joeavaikath Joeavaikath deleted the tests branch July 7, 2025 18:15
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.

non-admin backup create

4 participants