State export log ignores some log files #3382

MDrakos · 2024-06-28T22:30:53Z

DX-2892 `state export log -i` should not consider logs for `state export log`

One thing to note about this PR is that moving the logging of arguments to the captain package means that they won't be at the top of the log file anymore. I still think it makes sense to log the arguments in that package. The added requirement of only logging known commands and flags means that we have to log after command registration so that we can inspect the arguments and omit things we don't recognize.

Naatan · 2024-07-10T21:01:58Z

internal/captain/command.go

@@ -880,3 +918,30 @@ func childCommands(cmd *Command) string {

 	return fmt.Sprintf("\n\nAvailable Commands:\n%s", table.Render())
 }
+
+func (c *Command) LogArgs() {
+	children, err := c.FindChildren(os.Args[1:])


We should use c.Args. I think for those it already dropped the first entry of os.Args, but worth verifying.

All of the arg values in the command appear to be the known command arguments and not the os.Args. We set the args as part of the cobra command but I'm not finding a way to get them back out.

Naatan · 2024-07-10T21:05:00Z

internal/captain/command.go

@@ -880,3 +918,30 @@ func childCommands(cmd *Command) string {

 	return fmt.Sprintf("\n\nAvailable Commands:\n%s", table.Render())
 }
+
+func (c *Command) LogArgs() {
+	children, err := c.FindChildren(os.Args[1:])


I'm pretty sure FindChild() already achieves the desired effect you're looking for. And it passes through cobra, guaranteeing that we get the right command. From there I think you could use command.cobra.CommandPath(), which should give you the full path.

You're right and it also turns out we have other ways to accomplish this already so I've deleted the FindChildren function.

Naatan · 2024-07-10T21:08:51Z

internal/runners/export/log.go

+				logging.Error("failed to validate log file: %s", err.Error())
+				continue


Bubble up errors please. This shouldn't ever cause an error, so if it does something is broken and we need to fix it. Logging just obfuscates the breakage.

Naatan · 2024-07-10T21:09:51Z

internal/runners/export/log.go

+	}
+	defer file.Close()
+
+	regex := regexp.MustCompile(`.*\] Args: \[(.*?)\], Flags: \[.*?\]`)


Suggested change

regex := regexp.MustCompile(`.*\] Args: \[(.*?)\], Flags: \[.*?\]`)

regex := regexp.MustCompile(`Args: \[(.*?)\], Flags: \[.*?\]`)

Doesn't seem necessary for this function to have an opinion on what the prefix looks like.

Naatan · 2024-07-10T21:12:29Z

internal/runners/export/log.go

+		}
+	}
+
+	if strings.Contains(args, "export") && strings.Contains(args, "log") {


I'd prefer we be a but more explicit with the matches. However unlikely, it's easy enough to guard against something like state exec -- myapp -i file.log --export or something along those lines.

I made the check sequential. Is there more that you're looking for?

Naatan · 2024-07-17T21:30:45Z

internal/captain/command.go

@@ -920,18 +885,15 @@ func childCommands(cmd *Command) string {
 }

 func (c *Command) LogArgs() {
-	children, err := c.FindChildren(os.Args[1:])
+	child, err := c.FindChild(os.Args[1:])


#3382 (comment)

For some reason all of my comments were Pending 🤦

internal/runners/export/log.go

MDrakos · 2024-07-08T23:21:40Z

internal/captain/command.go

+func flags() []string {
+	flags := []string{}
+	for _, arg := range os.Args {
+		if strings.HasPrefix(arg, "-") {
+			flags = append(flags, arg)
+		}
+	}
+	return flags
+}


This function is largely duplicated from the analytics package. I tried moving things around but ended up with import cycles so opted to put this here.

MDrakos · 2024-07-17T18:11:00Z

internal/runners/export/log.go

+		}
+	}
+
+	if strings.Contains(args, "export") && strings.Contains(args, "log") {


I made the check sequential. Is there more that you're looking for?

MDrakos · 2024-07-17T18:12:27Z

internal/captain/command.go

@@ -880,3 +918,30 @@ func childCommands(cmd *Command) string {

 	return fmt.Sprintf("\n\nAvailable Commands:\n%s", table.Render())
 }
+
+func (c *Command) LogArgs() {
+	children, err := c.FindChildren(os.Args[1:])


All of the arg values in the command appear to be the known command arguments and not the os.Args. We set the args as part of the cobra command but I'm not finding a way to get them back out.

MDrakos · 2024-07-17T18:13:05Z

internal/captain/command.go

@@ -880,3 +918,30 @@ func childCommands(cmd *Command) string {

 	return fmt.Sprintf("\n\nAvailable Commands:\n%s", table.Render())
 }
+
+func (c *Command) LogArgs() {
+	children, err := c.FindChildren(os.Args[1:])


You're right and it also turns out we have other ways to accomplish this already so I've deleted the FindChildren function.

Co-authored-by: Nathan Rijksen <nathanr@activestate.com>

Naatan · 2024-07-17T22:10:01Z

@MDrakos can't respond to actual comment cause it's "old" and github UX is awful.

Suggest instead passing os.Args to the LogArgs function, that way captain isn't violating its own responsibilities by reaching out to os.Args.

That said, perhaps it would be better to just log args internally when the command is executed and the args are passed, ie. here:

cli/internal/captain/command.go

Line 216 in 26129e4

func (c *Command) Execute(args []string) error {

Naatan

Looks good! I think we just need a simple integration test covering this mechanic though, come to think of it. As this feels a little fragile considering how the logic is somewhat implicitly shared between different packages.

Naatan

Nice work!

State export log ignores some log files

8523e71

MDrakos added the Test: export label Jun 28, 2024

github-actions bot changed the base branch from master to version/0-46-0-RC1 June 28, 2024 22:31

MDrakos added 5 commits June 28, 2024 15:40

Add cmd to populate logs

2eb1fe1

Add comment

bd795b7

Don't fail if we can't validate log file

def4a03

Simplify slice initialization

0e33c7a

Wrap error

a06e793

MDrakos marked this pull request as ready for review July 9, 2024 21:50

MDrakos requested a review from Naatan July 9, 2024 21:50

Naatan requested changes Jul 10, 2024

View reviewed changes

MDrakos added 5 commits July 10, 2024 15:39

Use existing functions to find children

0e78bb6

Bubble up error

07a4ddc

Update regex

0c9392d

Simplify args construction further

91f6fae

Ensure sequential 'export log'

6ef1064

MDrakos requested a review from Naatan July 17, 2024 18:13

Naatan requested changes Jul 17, 2024

View reviewed changes

MDrakos commented Jul 17, 2024

View reviewed changes

MDrakos and others added 2 commits July 17, 2024 15:00

Update internal/runners/export/log.go

99449c6

Co-authored-by: Nathan Rijksen <nathanr@activestate.com>

Merge branch 'version/0-46-0-RC1' into DX-2892

b003d0f

MDrakos added 3 commits July 17, 2024 15:13

Log args internally

1d25de4

Merge branch 'version/0-46-0-RC1' into DX-2892

b0400d4

Fix logArgs slice

340f3bd

MDrakos requested a review from Naatan July 17, 2024 22:41

Naatan requested changes Jul 18, 2024

View reviewed changes

Add explicit integration test

6d00a35

MDrakos requested a review from Naatan July 29, 2024 17:47

Naatan approved these changes Jul 29, 2024

View reviewed changes

MDrakos merged commit dd0c713 into version/0-46-0-RC1 Jul 29, 2024
7 checks passed

MDrakos deleted the DX-2892 branch July 29, 2024 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State export log ignores some log files #3382

State export log ignores some log files #3382

MDrakos commented Jun 28, 2024 •

edited by github-actions bot

Loading

Naatan Jul 10, 2024

MDrakos Jul 17, 2024

Naatan Jul 10, 2024

MDrakos Jul 17, 2024

Naatan Jul 10, 2024

Naatan Jul 10, 2024

Naatan Jul 10, 2024

MDrakos Jul 17, 2024

Naatan Jul 17, 2024

MDrakos Jul 17, 2024

MDrakos Jul 8, 2024

MDrakos Jul 17, 2024

MDrakos Jul 17, 2024

MDrakos Jul 17, 2024

Naatan commented Jul 17, 2024

Naatan left a comment

Naatan left a comment

		logging.Error("failed to validate log file: %s", err.Error())
		continue

	regex := regexp.MustCompile(`.\] Args: \[(.?)\], Flags: \[.*?\]`)
	regex := regexp.MustCompile(`Args: \[(.?)\], Flags: \[.?\]`)

State export log ignores some log files #3382

State export log ignores some log files #3382

Conversation

MDrakos commented Jun 28, 2024 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan commented Jul 17, 2024

Naatan left a comment

Choose a reason for hiding this comment

Naatan left a comment

Choose a reason for hiding this comment

MDrakos commented Jun 28, 2024 •

edited by github-actions bot

Loading