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

Add suppress-logs flag #4530

Merged
merged 3 commits into from
Jul 24, 2020
Merged

Conversation

MarlonGamez
Copy link
Contributor

Fixes: #4512
Related: #4514, #4515, #4516

Description
This PR simply adds a --suppress-logs flag. It currently has no functionality, but this will be addressed by #4514, #4515, and #4516.

Follow-up Work
The 3 previously mentioned issues will need to be addressed in order to provide functionality.

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #4530 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4530      +/-   ##
==========================================
+ Coverage   72.35%   72.61%   +0.25%     
==========================================
  Files         333      335       +2     
  Lines       12959    12963       +4     
==========================================
+ Hits         9377     9413      +36     
+ Misses       2984     2957      -27     
+ Partials      598      593       -5     
Impacted Files Coverage Δ
pkg/skaffold/config/options.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/flags.go 86.36% <100.00%> (ø)
pkg/skaffold/kubernetes/client.go 0.00% <0.00%> (-40.00%) ⬇️
pkg/skaffold/kubernetes/context/context.go 85.36% <0.00%> (-12.20%) ⬇️
pkg/skaffold/deploy/labels.go 49.36% <0.00%> (-6.59%) ⬇️
pkg/skaffold/deploy/kubectl/labels.go 87.09% <0.00%> (-6.01%) ⬇️
pkg/skaffold/update/update.go 53.57% <0.00%> (-5.81%) ⬇️
pkg/skaffold/schema/util/util.go 76.66% <0.00%> (-5.16%) ⬇️
pkg/skaffold/deploy/deploy_mux.go 87.80% <0.00%> (-1.33%) ⬇️
cmd/skaffold/app/cmd/cmd.go 64.96% <0.00%> (-1.22%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd7cc9a...acdab49. Read the comment docs.

@MarlonGamez MarlonGamez requested review from a team and gsquared94 and removed request for a team July 22, 2020 20:33
@MarlonGamez MarlonGamez marked this pull request as ready for review July 22, 2020 20:33
@MarlonGamez MarlonGamez requested a review from a team as a code owner July 22, 2020 20:33
@gsquared94
Copy link
Contributor

I saw you mentioned somewhere that we can't repurpose --quiet. Not exactly sure why though.

@MarlonGamez
Copy link
Contributor Author

MarlonGamez commented Jul 22, 2020

I saw you mentioned somewhere that we can't repurpose --quiet. Not exactly sure why though.

I think our goal with this flag is to be able to use it to specify which stages we want to suppress the output of. For example, I would run skaffold run --suppress-logs=deploy or maybe skaffold dev --suppress-logs=build,deploy.

Currently, --quiet exists just as a boolean flag for skaffold build, so if we repurpose it to function the way I mentioned above, we risk breaking users who are currently using skaffold build --quiet. They'd have to change their commands to use skaffold build --quiet=build. I talked to @nkubala about this and we decided it's probably best to introduce a new flag.

@@ -368,6 +368,14 @@ var FlagRegistry = []Flag{
FlagAddMethod: "BoolVar",
DefinedOn: []string{"render"},
},
{
Name: "suppress-logs",
Usage: "Suppress logs for specified stages in pipeline (build, deploy, status-check)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to suppress them or shorten them?
Could it just be a "--verbose-logs" that's on by default? I feel it's a bit overkill to be able to change the logs for each stage independently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgageot I do think that shorten is probably a better word to use over suppress. As for being able to control each stage independently, do you think that it's overkill from a user's perspective or from an implementation perspective? If from a user's perspective, I think we could support them using =none or =all in order to simplify things. This would allow them to not have to deal with per stage settings

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's overkill for the users. But I'm open to other opinions :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GoogleContainerTools/skaffold-team anyone have thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

We talked about this in the logging breakout yesterday and it does seem useful to be able to suppress logs from different phases when the phase is successful. This would be helpful for the Cloud Run emulation situation where Skaffold is being used as the mechanism for local building and deploying, but ideally shouldn't be shown to the user.

Copy link
Member

Choose a reason for hiding this comment

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

In that situation, we might want to show the build logs but hide the deploy and status checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that then

@MarlonGamez MarlonGamez merged commit 33b1d10 into GoogleContainerTools:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configurable mode for suppressing verbose logs
5 participants