Skip to content

CLOUDP-60954: mongocli om logs collect #123

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

Merged
merged 4 commits into from
Apr 24, 2020
Merged

Conversation

andreaangiolillo
Copy link
Collaborator

@andreaangiolillo andreaangiolillo commented Apr 24, 2020

Proposed changes

Jira ticket: CLOUDP-60954

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

./bin/mongocli om logs collect replicaset  myReplicaSet --sizeRequestedPerFileBytes 64 --logTypes AUTOMATION_AGENT --profile OPS

{
	"id": "5ea2c9dd2f3b443e64fb727d"
}

"github.com/mongodb/mongocli/internal/config"
)

type LogsDownloader interface {
DownloadLog(string, string, string, io.Writer, *atlas.DateRangetOptions) error
}

type Logs interface {
Collect(string, *om.LogCollectionJob) (*om.LogCollectionJob, error)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to change its name. Maybe to CreateLogJob

Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

Couple of nits here and there but great initial work

ResourceType = "Type of resource from which to collect logs."
LogTypes = "Array of strings specifying the types of logs to collect."
SizeRequestedPerFileBytes = "Size for each log file in bytes."
Redacted = "If set to true, emails, hostnames, IP addresses, and namespaces in API responses involving this job are replaced with random string values."
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Suggested change
Redacted = "If set to true, emails, hostnames, IP addresses, and namespaces in API responses involving this job are replaced with random string values."
LogRedacted = "If set to true, emails, hostnames, IP addresses, and namespaces in API responses involving this job are replaced with random string values."

Or RedactedLog

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you want me to rename also SizeRequestedPerFileBytes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you, I think that one is quite unique for this command but redacted may show up in other commands and we may use it without realising it has log copy in it and not something generic

@andreaangiolillo andreaangiolillo requested a review from gssbzn April 24, 2020 11:30
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

couple of copy changes and then we are good, also to clarify something
things in the flags package are ok to be generic, things like type, redacted, start, end are probably going to be used in more than one command but things in the usage package need to either be generic as well or specific of the command, if we include copy like log, measurements or similar in a generic Type usage we risk that someone reusing the constant shows the wrong usage for a different command (this happened to me the other day reusing start and end)

func OpsManagerLogsCollectOptsBuilder() *cobra.Command {
opts := &opsManagerLogsCollectOpts{}
cmd := &cobra.Command{
Use: "collect",
Copy link
Collaborator

Choose a reason for hiding this comment

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

smal nit

Suggested change
Use: "collect",
Use: "collect [resourceType] [resourceName]",


args[0] = strings.ToLower(args[0])
if !search.StringInSlice(cmd.ValidArgs, args[0]) {
return fmt.Errorf("invalid Resource Type. The Resource Type must be cluster, process or replicaset but was %q", args[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need that many capital letters here, they are not proper nouns

Suggested change
return fmt.Errorf("invalid Resource Type. The Resource Type must be cluster, process or replicaset but was %q", args[0])
return fmt.Errorf("invalid resource type '%s', expected one of %v", args[0], cmd. ValidArgs)

@@ -115,4 +115,7 @@ const (
Normalization = "normalization" // Normalization flag
Backwards = "backwards" // Backwards flag
Strength = "strength" // Strength flag
LogTypes = "logTypes" // LogTypes flag
SizeRequestedPerFileBytes = "sizeRequestedPerFileBytes" //SizeRequestedPerFileBytes flag
LogRedacted = "redacted" // LogRedacted flag
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could have stayed Redacted, there's no log in the value so anyone should be free to use it

@@ -115,4 +115,7 @@ const (
Normalization = "normalization" // Normalization flag
Backwards = "backwards" // Backwards flag
Strength = "strength" // Strength flag
LogTypes = "logTypes" // LogTypes flag
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's already a Type const in this file, let's use that one.
we don't need the log part in the flag since are already in a log command

@andreaangiolillo andreaangiolillo requested a review from gssbzn April 24, 2020 12:19
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the fixes

@andreaangiolillo andreaangiolillo merged commit d6b839b into master Apr 24, 2020
@andreaangiolillo andreaangiolillo deleted the CLOUDP-60954 branch April 24, 2020 12:29
corryroot pushed a commit to corryroot/mongodb-atlas-cli that referenced this pull request Jun 15, 2023
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.

2 participants