NO-JIRA: Include the target version in the logging #2050
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hongkailiu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
added
the
needs-rebase
hongkailiu
force-pushed
the
target-version
branch
from
41608bd to
c08af1a
Compare
openshift-merge-robot
removed
the
needs-rebase
hongkailiu
force-pushed
the
target-version
branch
from
c08af1a to
ae3a133
Compare
Before this full, the logging was only for the case that `findClusterIncludeConfigFromInstallConfig` is called, i.e., the path from an install-config file is provided. This pull extends it to the case where the configuration is taken from the current cluster. Another change from the pull is that the logging messages include the target version that is determined by inspecting the release image. The implementation for this is adding a new callback `ImageConfigCallback`.
hongkailiu
force-pushed
the
target-version
branch
from
ae3a133 to
7056f53
Compare
|
/cc |
| inclusionConfig, err = findClusterIncludeConfig(ctx, o.RESTConfig, versionInImageConfig) | ||
| } else { | ||
| inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig) | ||
| inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig, versionInImageConfig) |
There was a problem hiding this comment.
Maybe I am missing something but I don't understand when is versionInImageConfig actually populated. What I'm reading is:
- It is empty on L354 when declared
- It can be populated on L371 inside the
opts.ImageConfigCallbackcallback (which itself is populated on L355) - It can only be actually populated once
opts.ImageConfigCallbackis called at least one - The callback is called in
pkg/cli/image/extract/extract.go:L426ino.Run() - Here on L384 / L386 we expect this to be a populated string already but I do not see any opportunity betwen L372 and the L383
ifwhereo.Run()could be called, even indirectly.
How is versionInImageConfig not always an empty string on L384/L386??
There was a problem hiding this comment.
This is reiterated #1958 (comment) but I do not understand the answer.
| return | ||
| } | ||
| klog.V(2).Infof("Retrieved the version from image configuration in the image to extract: %s", v) | ||
| versionInImageConfig = v |
There was a problem hiding this comment.
We assume we never reach this place twice here (= we assume the callback is never called multiple time in a way that reaches this assignment) otherwise the old value is overwritten.
That assumption may be fine but I'd either add a comment here about it or logged if we reach here while versionInImageConfig is not empty.
| @@ -349,6 +349,28 @@ func (o *ExtractOptions) Run(ctx context.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| // versionInImageConfig stores the version from the label of the image | |||
| // At the moment it is used later only for the logging purpose and thus is not blocking if failures occur upon getting its value | |||
There was a problem hiding this comment.
If the errors are not blocking then maybe they should be logged as warnings, not errors?
|
@hongkailiu: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Before this full, the logging was only for the case that
findClusterIncludeConfigFromInstallConfigis called, i.e., the path from an install-config file is provided.This pull extends it to the case where the
configuration is taken from the current cluster.
Another change from the pull is that the logging
messages include the target version that is determined by inspecting the release image. The implementation for this is adding a new callback
ImageConfigCallback.