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

Expose --event-log-file to render, apply, and test #5828

Merged

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented May 12, 2021

Fixes #5831 #5833 #5834
Related to: #5125

Description
#5125 added the ability to log events to a file for dev, build, run, debug, deploy. It would be useful to have this available for render, apply, and test.

This PR adds --event-log-file and --enable-rpc for render, apply, and test. It also marks the flag as hidden: it was intended as an internal tool for integration tests:

Follow-up Work (remove if N/A)

  • it would be useful to remove the requirement to also run --enable-rpc
  • integration tests to ensure that this flag is available as partners may rely on it

@briandealwis briandealwis requested a review from a team as a code owner May 12, 2021 22:05
@briandealwis briandealwis requested a review from nkubala May 12, 2021 22:05
@google-cla google-cla bot added the cla: yes label May 12, 2021
@aaron-prindle
Copy link
Contributor

Code change LGTM, I believe doc generation needs to be re-run here - ./hack/generate-man.sh. From travis logs

/hack/man/TestPrintMan
    man_test.go:52: You have skaffold command changes but haven't generated the CLI reference docs. Please run ./hack/generate-man.sh and commit the results!

@aaron-prindle aaron-prindle requested review from aaron-prindle and removed request for nkubala May 12, 2021 22:36
@briandealwis
Copy link
Member Author

Docs regenerated @aaron-prindle

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #5828 (7407737) into master (35b72aa) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5828      +/-   ##
==========================================
- Coverage   70.99%   70.95%   -0.05%     
==========================================
  Files         440      440              
  Lines       16572    16582      +10     
==========================================
  Hits        11765    11765              
- Misses       3939     3949      +10     
  Partials      868      868              
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 89.28% <ø> (ø)
pkg/skaffold/debug/debug.go 32.83% <0.00%> (-5.77%) ⬇️

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 35b72aa...7407737. Read the comment docs.

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Like we discussed in the standup, please mark the flag hidden.

@briandealwis
Copy link
Member Author

Flag is now hidden.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

LGTM, I restarted the flaked CI job but feel free and merge when green

@nkubala nkubala enabled auto-merge (squash) May 13, 2021 18:54
@tejal29 tejal29 dismissed their stale review May 13, 2021 18:59

addressed

@nkubala nkubala merged commit ffb7d2b into GoogleContainerTools:master May 13, 2021
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.

Event logfile support for skaffold render
4 participants