Command Summary - Invoke summary generation from the CLI#2653
Merged
EyalDelarea merged 26 commits intojfrog:devfrom Aug 28, 2024
Merged
Command Summary - Invoke summary generation from the CLI#2653EyalDelarea merged 26 commits intojfrog:devfrom
EyalDelarea merged 26 commits intojfrog:devfrom
Conversation
4 tasks
2 tasks
4 tasks
eyalbe4
suggested changes
Aug 26, 2024
general/summary/cli.go
Outdated
| } | ||
|
|
||
| // Creates a final summary of recorded CLI commands that were executed on the current machine. | ||
| // The summary is generated in Markdown format and saved in the root directory of JFROG_CLI_COMMAND_SUMMARY_OUTPUT_DIR. |
Contributor
There was a problem hiding this comment.
...saved in the root directory of JFROG_CLI_COMMAND_SUMMARY_OUTPUT_DIR.
-->
...saved in the directory stored in the JFROG_CLI_COMMAND_SUMMARY_OUTPUT_DIR environment variabke.
general/summary/cli.go
Outdated
Comment on lines
+44
to
+45
| return fmt.Errorf("cannot generate command summary: the output directory for command recording is not defined. "+ | ||
| "Please set the environment variable %s before executing your commands to view their summary", coreutils.OutputDirPathEnv) |
Contributor
There was a problem hiding this comment.
Let's improve this message to be more user friendly.
How about -
return fmt.Errorf("unable to generate the command summary because the output directory is not specified. Please ensure that the environment variable '%s' is set before running your commands to enable summary generation.", coreutils.OutputDirPathEnv)
Also note that coreutils.OutputDirPathEnv is too generic of a description this variable.
Let's use coreutils.SummaryOutputDirPathEnv.
general/summary/cli.go
Outdated
| // Get URL and Version to generate summary links | ||
| serverUrl, majorVersion, err := extractServerUrlAndVersion(c) | ||
| if err != nil { | ||
| log.Warn("Failed to get server URL or major version: %v. This means markdown URLs will be invalid!", err) |
Contributor
There was a problem hiding this comment.
Why are we not returning the error, and just adding a warning message?
We need a very good reason to ignore error messages. Let's discuss this.
general/summary/cli.go
Outdated
| return platformDetails, nil | ||
| } | ||
|
|
||
| func extractServerUrlAndVersion(c *cli.Context) (string, int, error) { |
Contributor
There was a problem hiding this comment.
Let's used named return values, to make the code easier to read.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
devbranch.go vet ./....go fmt ./....Command Summary Generation Shifted to CLI
Previous Approach
The
setup-jfrog-cliGitHub action was responsible for collecting markdown data and generating a single summary.New Approach
The CLI now directly invokes each summary implementation’s markdown generation function, combining all outputs into a unified markdown summary.
Benefits:
New Command
jf generate-summary-markdownDepends on:
jfrog/jfrog-cli-core#1238
jfrog/setup-jfrog-cli#186