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
Bump K8s.io version to v0.19.0 #4266
Conversation
* Bump k8s.io modules version to v0.19.0 * Change machine-config-operator to latest master * Move from k8s.io/klog to k8s.io/klog/v2
/test e2e-crc |
.gitignore
Outdated
*.swp | ||
*dev.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the installer repo needs to add user-specific files to git ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the dev.log, will drop it
Regarding the *.swp, this is temporary file used by the vi/vim editor and might be accidentally added to commits. as this type file never should be added, isn't it good enough reason to add it (and maybe also other editor's specific files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people should use https://stackoverflow.com/a/1753078 to add any editor ignore files. Forcing repos to include ignore files for all editors is too much of a slippery slope imo, so i would personally not add anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abhinavdahiya
I removed this change
cmd/openshift-install/main.go
Outdated
"k8s.io/klog" | ||
klogv2 "k8s.io/klog/v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the vendor update doesn't reflect that klog was dropped or not used anymore by anyone.
so how can we ensure no logs end up into stderr from both versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use here both klog versions, do you think this is better approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use here both klog versions, do you think this is better approach?
i think that would be safer for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abhinavdahiya
I changed it, please review
/test e2e-ovirt |
/test e2e-metal-ipi |
/test e2e-ovirt |
/test e2e-aws |
@nirarg: 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/test-infra repository. I understand the commands that are listed here. |
cmd/openshift-install/main.go
Outdated
@@ -28,8 +29,10 @@ func main() { | |||
// to log anything. | |||
var fs flag.FlagSet | |||
klog.InitFlags(&fs) | |||
klogv2.InitFlags(&fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the same flag set for both v1 and v2 is causing this failure https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_installer/4266/pull-ci-openshift-installer-master-e2e-aws/1316299561756528640#1:build-log.txt%3A129
…le all logs from k8s clients
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@nirarg: 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/test-infra repository. I understand the commands that are listed here. |
No description provided.