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: emit metrics for exec, verify and render #9078

Merged
merged 3 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions cmd/skaffold/app/cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
if cmd.Name() != cobra.ShellCompRequestCmd && cmd.Name() != cobra.ShellCompNoDescRequestCmd {
instrumentation.SetCommand(cmd.Name())
out := output.GetWriter(context.Background(), out, defaultColor, forceColors, timestamps)
cmd.Root().SetOutput(out)
cmd.Root().SetOut(out)
cmd.Root().SetErr(errOut)

// Setup logs
if err := setUpLogs(errOut, v, timestamps); err != nil {
Expand Down Expand Up @@ -131,21 +132,21 @@ func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
if err := config.UpdateMsgDisplayed(opts.GlobalConfig); err != nil {
log.Entry(context.TODO()).Debugf("could not update the 'last-prompted' config for 'update-config' section due to %s", err)
}
fmt.Fprintf(cmd.OutOrStderr(), "%s\n", msg)
fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", msg)
default:
}
// check if survey prompt needs to be displayed
select {
case promptSurveyID := <-surveyPrompt:
if promptSurveyID != "" {
if err := s.DisplaySurveyPrompt(cmd.OutOrStdout(), promptSurveyID); err != nil {
if err := s.DisplaySurveyPrompt(output.NewColorWriter(cmd.ErrOrStderr()), promptSurveyID); err != nil {
fmt.Fprintf(cmd.OutOrStderr(), "%v\n", err)
}
}
default:
}
if metricsPrompt {
if err := prompt.DisplayMetricsPrompt(opts.GlobalConfig, cmd.OutOrStdout()); err != nil {
if err := prompt.DisplayMetricsPrompt(opts.GlobalConfig, output.NewColorWriter(cmd.ErrOrStderr())); err != nil {
fmt.Fprintf(cmd.OutOrStderr(), "%v\n", err)
}
}
Expand Down
1 change: 1 addition & 0 deletions cmd/skaffold/app/cmd/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func NewCmdExec() *cobra.Command {
WithExample("Execute a defined action that uses an image built from Skaffold. First, build the images", "build --file-output=build.json").
WithExample("Then use the built artifacts", "exec <action-name> --build-artifacts=build.json").
WithCommonFlags().
WithHouseKeepingMessages().
WithArgs(func(cmd *cobra.Command, args []string) error {
if len(args) != 1 {
log.Entry(context.TODO()).Errorf("`exec` requires exactly one action to execute")
Expand Down
1 change: 1 addition & 0 deletions cmd/skaffold/app/cmd/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func NewCmdRender() *cobra.Command {
// This "--output" flag replaces the --render-output flag, which is deprecated.
{Value: &opts.RenderOutput, Name: "output", Shorthand: "o", DefValue: "", Usage: "File to write rendered manifests to"},
}).
WithHouseKeepingMessages().
NoArgs(doRender)
}

Expand Down
2 changes: 1 addition & 1 deletion hack/versions/pkg/schema/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"github.com/GoogleContainerTools/skaffold/v2/hack/versions/pkg/diff"
)

const baseRef = "origin/main"
const baseRef = "origin/release/v2.6"

func RunSchemaCheckOnChangedFiles() error {
git, err := newGit(baseRef)
Expand Down
3 changes: 2 additions & 1 deletion integration/housekeeping_messages_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func TestHouseKeepingMessagesNotShownForDiagnose(t *testing.T) {
func TestHouseKeepingMessagesShownForDev(t *testing.T) {
MarkIntegrationTest(t, CanRunWithoutGcp)
file := testutil.TempFile(t, "config", nil)
out := skaffold.Run("-c", file, "--update-check=true").InDir("examples/getting-started").RunOrFailOutput(t)
out, err := skaffold.Run("-c", file, "--update-check=true").InDir("examples/getting-started").RunWithCombinedOutput(t)
testutil.CheckError(t, false, err)
testutil.CheckContains(t, "Help improve Skaffold", string(out))
skaffold.Delete().InDir("examples/getting-started").Run(t)
}
2 changes: 1 addition & 1 deletion pkg/skaffold/instrumentation/meter.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ var (
)

func init() {
MeteredCommands.Insert("apply", "build", "delete", "deploy", "dev", "debug", "filter", "generate_pipeline", "render", "run", "test")
MeteredCommands.Insert("apply", "build", "delete", "deploy", "dev", "debug", "filter", "generate_pipeline", "render", "run", "test", "verify", "exec")
doesBuild.Insert("build", "render", "dev", "debug", "run")
doesDeploy.Insert("apply", "deploy", "dev", "debug", "run")
}
Expand Down
8 changes: 2 additions & 6 deletions pkg/skaffold/instrumentation/prompt/prompt.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ You may choose to opt out of this collection by running the following command:

var (
// for testing
isStdOut = output.IsStdout
updateConfig = config.UpdateGlobalCollectMetrics
getConfig = config.GetConfigForCurrentKubectx
setStatus = instrumentation.SetOnlineStatus
Expand All @@ -53,9 +52,6 @@ func ShouldDisplayMetricsPrompt(configfile string) bool {
}

func DisplayMetricsPrompt(configFile string, out io.Writer) error {
if isStdOut(out) {
output.Green.Fprintf(out, Prompt)
return updateConfig(configFile, true)
}
return nil
output.Green.Fprintf(out, Prompt)
return updateConfig(configFile, true)
}
16 changes: 4 additions & 12 deletions pkg/skaffold/instrumentation/prompt/prompt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package prompt
import (
"bytes"
"fmt"
"io"
"testing"

"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config"
Expand Down Expand Up @@ -74,23 +73,16 @@ func TestShouldDisplayMetricsPrompt(t *testing.T) {

func TestDisplayMetricsPrompt(t *testing.T) {
tests := []struct {
name string
mockStdOut bool
expected string
name string
expected string
}{
{
name: "std out",
mockStdOut: true,
expected: Prompt,
},
{
name: "not std out",
name: "std out",
expected: Prompt,
},
}
for _, test := range tests {
testutil.Run(t, test.name, func(t *testutil.T) {
mock := func(io.Writer) bool { return test.mockStdOut }
t.Override(&isStdOut, mock)
t.Override(&updateConfig, func(_ string, _ bool) error { return nil })
var buf bytes.Buffer
err := DisplayMetricsPrompt("", &buf)
Expand Down
13 changes: 0 additions & 13 deletions pkg/skaffold/output/output.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package output
import (
"context"
"io"
"os"
"time"

"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants"
Expand Down Expand Up @@ -76,18 +75,6 @@ func GetWriter(ctx context.Context, out io.Writer, defaultColor int, forceColors
}
}

func IsStdout(out io.Writer) bool {
sw, isSW := out.(skaffoldWriter)
if isSW {
out = sw.MainWriter
}
cw, isCW := out.(colorableWriter)
if isCW {
out = cw.Writer
}
return out == os.Stdout
}

// GetUnderlyingWriter returns the underlying writer if out is a colorableWriter
func GetUnderlyingWriter(out io.Writer) io.Writer {
sw, isSW := out.(skaffoldWriter)
Expand Down
46 changes: 0 additions & 46 deletions pkg/skaffold/output/output_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,52 +30,6 @@ import (
"github.com/GoogleContainerTools/skaffold/v2/testutil"
)

func TestIsStdOut(t *testing.T) {
tests := []struct {
description string
out io.Writer
expected bool
}{
{
description: "std out passed",
out: os.Stdout,
expected: true,
},
{
description: "out nil",
out: nil,
},
{
description: "out bytes buffer",
out: new(bytes.Buffer),
},
{
description: "colorable std out passed",
out: skaffoldWriter{
MainWriter: NewColorWriter(os.Stdout),
},
expected: true,
},
{
description: "colorableWriter passed",
out: NewColorWriter(os.Stdout),
expected: true,
},
{
description: "invalid colorableWriter passed",
out: skaffoldWriter{
MainWriter: NewColorWriter(io.Discard),
},
expected: false,
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.CheckDeepEqual(test.expected, IsStdout(test.out))
})
}
}

func TestGetUnderlyingWriter(t *testing.T) {
tests := []struct {
description string
Expand Down
4 changes: 0 additions & 4 deletions pkg/skaffold/survey/survey.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ Tip: To permanently disable the survey prompt, run:

var (
// for testing
isStdOut = output.IsStdout
open = browser.OpenURL
updateSurveyPrompted = sConfig.UpdateGlobalSurveyPrompted
parseConfig = schema.ParseConfigAndUpgrade
Expand Down Expand Up @@ -109,9 +108,6 @@ func recentlyPrompted(gc *sConfig.SurveyConfig) bool {
}

func (s *Runner) DisplaySurveyPrompt(out io.Writer, id string) error {
if !isStdOut(out) {
return nil
}
if sc, ok := getSurvey(id); ok {
output.Green.Fprintf(out, sc.prompt())
return updateSurveyPrompted(s.configFile)
Expand Down
11 changes: 0 additions & 11 deletions pkg/skaffold/survey/survey_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package survey
import (
"bytes"
"fmt"
"io"
"testing"
"time"

Expand All @@ -34,26 +33,16 @@ func TestDisplaySurveyForm(t *testing.T) {
description string
mockSurveyPrompted func(_ string) error
expected string
mockStdOut bool
}{
{
description: "std out",
mockStdOut: true,
mockSurveyPrompted: func(_ string) error { return nil },
expected: `Help improve Skaffold with our 2-minute anonymous survey: run 'skaffold survey'
`,
},
{
description: "not std out",
mockSurveyPrompted: func(_ string) error {
return fmt.Errorf("not expected to call")
},
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
mock := func(io.Writer) bool { return test.mockStdOut }
t.Override(&isStdOut, mock)
mockOpen := func(string) error { return nil }
t.Override(&open, mockOpen)
t.Override(&updateSurveyPrompted, test.mockSurveyPrompted)
Expand Down
Loading