[GKE] Enable Log dumping #59
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| @@ -38,6 +38,16 @@ func (d *deployer) Up() error { | |||
| return err | |||
| } | |||
|
|
|||
| defer func() { | |||
| if d.RepoRoot == "" { | |||
| klog.Warningf("repo-root not supplied, skip dumping cluster logs") | |||
There was a problem hiding this comment.
I feel this should be Info since it's normal for non-k8s projects to skip dumping clusters logs.
There was a problem hiding this comment.
Usually info comments are used for more verbose explanation of steps being carried out.
Whereas here we'll be missing a key part of the deployer lifecycle, feels like it should be a warning.
| @@ -9,16 +9,12 @@ import ( | |||
| type BuildOptions struct { | |||
| Strategy string `flag:"~strategy" desc:"Determines the build strategy to use either make or bazel."` | |||
| StageLocation string `flag:"~stage" desc:"Upload binaries to gs://bucket/ci/job-suffix if set"` | |||
| RepoRoot string `flag:"~repo-root" desc:"Path to root of the kubernetes repo. Used with --build."` | |||
|
|
|||
| RepoRoot string `flag:"-"` | |||
There was a problem hiding this comment.
Could you explain how - works here? Is this change related to the other RepoRoot field added in the deployer struct?
There was a problem hiding this comment.
Yup, - means this field will not be exposed as a flag: https://github.com/octago/sflags#options-for-flag-tag
now that the top level field is exposed.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Move repo-root to top-level since it's also required by dumplogs.
Add automatic log dumping on cluster create.