Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migrate CLI to use cobra #114

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

1garo
Copy link

@1garo 1garo commented Feb 27, 2024

Closes #112

@kehoecj kehoecj added the OSS Community Contribution Contributions from the OSS Community label Feb 27, 2024
@1garo 1garo marked this pull request as ready for review February 27, 2024 23:06
@1garo
Copy link
Author

1garo commented Feb 27, 2024

@kehoecj

The change seems to be working fine, I mostly was able to reproduce the same thing as Flag.
For sure the code need some trimming and improvements, but I feel that is functional.

If you could do some testing, I could be missing some corn-cases.

Due to some changes the install changed a little bit, as we will still discuss this, I will not change on the README:

CGO_ENABLED=0 \
GOOS=linux \
GOARCH=amd64 \
go build \
-ldflags='-w -s -extldflags "-static"' \
-tags netgo \
-o validator \
main.go

@1garo 1garo marked this pull request as draft February 27, 2024 23:08
@1garo
Copy link
Author

1garo commented Feb 28, 2024

This test case is returning exit status 1 instead of 0:
{"exclude file types set", []string{"--exclude-file-types=json", "."}, 0}

It is because in the "." path there is some "bad files", if I run the same command with the addition of --depth 1 it works fine (folder with just "good files"), what you guys think I could do in this case?

@1garo 1garo marked this pull request as ready for review March 1, 2024 17:57
@jd4235
Copy link
Contributor

jd4235 commented Mar 7, 2024

The lint job failed on the latest pipeline. Need to fix.

@1garo
Copy link
Author

1garo commented Mar 11, 2024

The lint job failed on the latest pipeline. Need to fix.

Fixed.

@kehoecj kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Mar 27, 2024
Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

A few items:

  • We had a few merges prior to this one. Please resolve all conflicts
  • During functional checkout I am having issues with generating a valid binary both on MacOS and in the Docker container.
>> docker run -v $(pwd)/test:/test cfv:cobra --depth 0 /test
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "/validator": permission denied: unknown.

and macos

>> CGO_ENABLED=0 \
> GOOS=darwin \
> GOARCH=amd64 \
> go build \
> -ldflags='-w -s -extldflags "-static"' \
> -tags netgo \
> -o validator \
> cmd/validator/validator.go
>> ./validator --version
-bash: ./validator: Permission denied

Main does not have this problem so I'm not sure if this is a byproduct of Cobra

@kehoecj kehoecj removed the waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers label Apr 10, 2024
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
PKGBUILD Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

@1garo Your PR did not properly resolve conflicts in your last update. I started to go through them all but you should get the idea on the rest. It looks like you rejected all upstream conflicts and just kept your code which is now trying to overwrite existing changes

@kehoecj
Copy link
Collaborator

kehoecj commented Apr 10, 2024

@1garo Why was -search_path added back rather than treating it as a positional argument? Search path should remain a positional argument

To clarify, this now fails:

>> docker run -v $(pwd)/test:/test cfv:cobra /test
Error: unknown command "/test" for "validator"
Run 'validator --help' for usage.
unknown command "/test" for "validator"

and instead I have to do this:

docker run -v $(pwd)/test:/test cfv:cobra --search_path /test

@kehoecj kehoecj added the pr-action-requested PR is awaiting feedback from the submitting developer label Apr 10, 2024
@1garo
Copy link
Author

1garo commented Apr 12, 2024

@1garo Your PR did not properly resolve conflicts in your last update. I started to go through them all but you should get the idea on the rest. It looks like you rejected all upstream conflicts and just kept your code which is now trying to overwrite existing changes

fixed at 8a5c705

@1garo Why was -search_path added back rather than treating it as a positional argument? Search path should remain a positional argument

To clarify, this now fails:

>> docker run -v $(pwd)/test:/test cfv:cobra /test
Error: unknown command "/test" for "validator"
Run 'validator --help' for usage.
unknown command "/test" for "validator"

and instead I have to do this:

docker run -v $(pwd)/test:/test cfv:cobra --search_path /test

fixed at fix: add search_path as positional argument

Comment on lines +8 to +12
Cross Platform tool to validate configuration files

Usage:
validator [flags]
validator [command]
Copy link
Author

Choose a reason for hiding this comment

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

You think we should maintain this comment here? Since running -h already outputs the usage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Community Contribution Contributions from the OSS Community pr-action-requested PR is awaiting feedback from the submitting developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor CLI code to use Cobra
3 participants