-
Notifications
You must be signed in to change notification settings - Fork 11
Feature/improve ux #50
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
Conversation
be987e2 to
d02017f
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this 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 improves the CLI’s user experience by displaying a welcome message when no configuration directory exists, providing enhanced help text when configuration is present, and incorporating a progress bar during installations. It also updates workflow configurations and cleans up output messages.
- Enhanced installation output and logging in the installer functions.
- Improved CLI commands with a welcome message and detailed help text.
- Updated CI workflow to reflect current supported OS environments.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| config/tools-installer.go | Added installation progress messages for each tool. |
| config/runtimes-installer.go | Commented out log messages for runtime downloads and extraction. |
| cmd/root.go | Enhanced CLI descriptions and added a welcome message function. |
| cmd/install.go | Introduced a progress bar for overall installation process. |
| cmd/init.go | Initiated configuration bootstrapping before token checks. |
| cli-v2.go | Removed legacy CLI functionality and simplified main flow. |
| .github/workflows/go.yml | Updated the CI workflow to exclude Windows and adjust build steps. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
cmd/install.go:68
- Verify that progressbar.OptionShowIts() is a valid option for the progressbar library; if it is not, consider removing or replacing it to ensure the progress bar behaves as expected.
progressbar.OptionShowIts(),
cmd/install.go
Outdated
| green := color.New(color.FgGreen) | ||
|
|
||
| // Initialize config | ||
| cfg.Init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the init as part of the install? I think as it is, probably a bad ideia. As it will not have the token on the install command and will override the versions with CLI defaults instead of what the user has access no?
Maybe sooner than later, we need to have the 'init' mechanism to store the token or something, that would be able to be used during the steps? Whatever usually CLIs like gh or wtv, that you stay 'logged in' and don't need to pass a token to every command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is Config object init, not cli init.
we were already doing it in main: https://github.com/codacy/codacy-cli-v2/pull/50/files#diff-5255824def31d434b7b0cd0d5befdda5e376565f53423f2f821844522fda4d46L11-L14
For now we can rename it or smth. In future refactor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I am a bit confused. Is something needed for all the commands? If so, you need to remove it from main and add it all commands?
Can you explain me why we need to remove it from main, and why some commands need it and others not?
| yellow.Println("To get started, you'll need a Codacy API token.") | ||
| fmt.Println("You can find your Project API token in Codacy under:") | ||
| fmt.Println("Project > Settings > Integrations > Repository API tokens") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use API token instead of Codacy API token. And then also expose the possibility of doing it with account API tokens as well, and specifying the URL directly.
This variant would be faster and more user friendly. To my understanding, it works with both types of API tokens, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also was thinking just go with "general" API token, not the project one.
Just for simplicity now and create more limited token in Future. I think the IDE plugin is using API token anway....
wdyt @machadoit
Anyway this will be resolved in another PR - this one is more UI specfifc - there is another Task for making setup flow easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API token instead of Codacy API token
Sorry what? 😅 Now it works with repository-token, and you are talking about the Account API token correct?
Yeah, if it is a requirement for the MCP, we can add a task to add support for it (but agree that is out of scope of this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created https://codacy.atlassian.net/browse/PLUTO-1379 for the follow up, please take a look @andrzej-janczak @manufacturist
| fmt.Println("Project > Settings > Integrations > Repository API tokens") | ||
| fmt.Println() | ||
| cyan.Println("Initialize your project with:") | ||
| fmt.Println(" codacy-cli init --repository-token YOUR_TOKEN") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --repository-token option is not specified in the README.md. Would this be a part of the scope of this improvement, or will it be treated within https://codacy.atlassian.net/browse/PLUTO-1376 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing missing is not just the --repository-token but the full init command and its options. Can be either here or later, as @andrzej-janczak prefer. (I will add the note on the task anyway)
- Removed configuration file check from main function, allowing direct command execution. - Improved installation command to include detailed logging for tool installation progress. - Added user-friendly messages during the installation process to indicate success and failures.
- Added functions to verify if runtimes and tools are already installed. - Improved installation process to skip already installed components and provide user-friendly messages. - Updated Codacy configuration to use the latest version of Trivy.
0835afe to
8aed5d8
Compare
- Renamed `codacyDirectory` to `globalCacheDirectory` for clarity. - Updated methods to create necessary directories during initialization and installation. - Removed the old directory creation function in favor of a more streamlined approach.
.github/workflows/go.yml
Outdated
| - name: Install dependencies from .codacy/codacy.yaml | ||
| run: | | ||
| go build ./cli-v2.go | ||
| ./cli-v2 init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can we do it without the init on the CI? The init is something that should be done 'once' and locally, and committed
cmd/init.go
Outdated
| Short: "Bootstraps project configuration", | ||
| Long: "Bootstraps project configuration, creates codacy configuration file", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| // Initialize configuration without creating directories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would remove the comment. As we create the .codacy folder as part of it
machadoit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing a couple things, but would like to go over them tomorrow
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| err = buildRepositoryConfigurationFiles(codacyRepositoryToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the repository configuration files with token option? 🤔 I guess we are moving it to the install? That seems strange / wrong. As the token should also be used to fetch the correct versions of runtimes and tools, no?
Can we make this as it was on the scope of this improvements / refactors?
| var codacyRepositoryToken string | ||
|
|
||
| func init() { | ||
| initCmd.Flags().StringVar(&codacyRepositoryToken, "repository-token", "", "optional codacy repository token, if defined configurations will be fetched from codacy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually something seems a bit off, we removed the var, but still exists here? 🤔
cmd/install.go
Outdated
|
|
||
| func init() { | ||
| installCmd.Flags().StringVarP(®istry, "registry", "r", "", "Registry to use for installing tools") | ||
| installCmd.Flags().StringVar(&codacyRepositoryToken, "repository-token", "", "Codacy repository token for fetching tool configurations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even see where this is used during installation, maybe it should be removed?
cmd/root.go
Outdated
| It provides functionality for code analysis, configuration management, | ||
| and integration with Codacy's services. | ||
| To get started, try running one of these commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to show when? Maybe this block can be removed, as the CLI already shows the available commands on the help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when run witout args go run cli-v2.go (codacy-cli)
AND when you have codacy yaml file
Yeah, this part is redundant with help so removing it
- Added informative messages to guide users when no project token is specified. - Included next steps for users after successful initialization of Codacy configuration.
| Short: "Codacy CLI", | ||
| Long: "Code analysis", | ||
| Short: "Codacy CLI - A command line interface for Codacy", | ||
| Long: ``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what? An empty line or?
machadoit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* feature: improve starting from zero UX * ux: update API token instructions in welcome message * chore: update GitHub Actions workflow to initialize cli-v2 and remove Windows support * feature: installation progress bar * feature: enhance installation checks and user feedback * refactor: update configuration handling and directory creation
* feature: improve starting from zero UX * ux: update API token instructions in welcome message * chore: update GitHub Actions workflow to initialize cli-v2 and remove Windows support * feature: installation progress bar * feature: enhance installation checks and user feedback * refactor: update configuration handling and directory creation
When there is no config dir Then welcome message is shown.
When there is BUT there is no command specified (
codacy-cli) Then--helpis shownAlso add progees bar 🚀