Skip to content

Fix broken kubepkg deb build test #1379

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 28, 2020
Merged

Fix broken kubepkg deb build test #1379

merged 2 commits into from
Jun 28, 2020

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Jun 26, 2020 •
edited
Loading

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

Two distinct issues have been fixed with this PR

  • Fix failing kubepkg debs test
    This fixes the failing kubepkg debs test which was failing because of
    the shelling-out mv command which resulted in a permission denied
    error. We fixed that by using the golang ioutils to copy the file
    around.

  • Fixed deb package def replacement for kubernetes-cni:
    kubernetes-cni is now provided by the kubelet package so we can remove
    it from the spec.

Which issue(s) this PR fixes:

Fixes kubernetes/kubernetes#88408

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

- Fixed kubepkg deb builds

@saschagrunert
Fix failing kubepkg debs test
4ab219d 
This fixes the failing `kubepkg debs` test which was failing because of
the shelling-out `mv` command which resulted in a permission denied
error. We fixed that by using the golang ioutils to copy the file
around.

Signed-off-by: Sascha Grunert <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2020
@saschagrunert saschagrunert changed the title Flaky test Fix broken kubepkg deb build test Jun 26, 2020
@k8s-ci-robot k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 26, 2020
@saschagrunert saschagrunert mentioned this pull request Jun 26, 2020
Closed
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 26, 2020
@justaugustus
Copy link
Member

FYI @hwdef (from your Slack DM)

justaugustus
justaugustus requested changes Jun 26, 2020
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

The deb/rpm specs for kubernetes-cni need to be restored, as well as the comment below.

pkg/kubepkg/kubepkg.go
@@ -587,6 +599,7 @@ func GetDependencies(packageDef *PackageDefinition) (map[string]string, error) {
deps["kubelet"] = minimumKubernetesVersion
deps["kubectl"] = minimumKubernetesVersion
deps["cri-tools"] = minimumCRIToolsVersion
deps["kubernetes-cni"] = MinimumCNIVersion // deb based kubeadm still requires kubernetes-cni
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to restore variable references in the package templates as well. They should roughly match what was removed in #1330.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, when I'm now thinking about this: the kubelet package already provides kubernetes-cni so we may remove it completely from the deb specs. I'll change the PR to propose this approach.

Copy link
Member

Choose a reason for hiding this comment

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

The packages should exactly match what's defined in packages/.
Let's just fix the tests here. No spec refactors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I reverted my previous change 👍

You'll need to restore variable references in the package templates as well. They should roughly match what was removed in #1330.

Which references do you mean? I'm not sure which paths needs further adaptions. 🤔

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020
saschagrunert
saschagrunert commented Jun 26, 2020
View reviewed changes
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2020
@saschagrunert
Fixed deb package def replacement for kubernetes-cni
2cd6f5c 
kubernetes-cni is now provided by the kubelet package so we can remove
it from the spec.

Signed-off-by: Sascha Grunert <[email protected]>
@justaugustus
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus, saschagrunert

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 2eea863 into kubernetes:master Jun 28, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 28, 2020
@saschagrunert saschagrunert deleted the flaky-test branch June 28, 2020 16:42
@justaugustus justaugustus mentioned this pull request Jun 28, 2020
Merged
@hwdef
Copy link
Member

hwdef commented Jun 29, 2020

@justaugustus @saschagrunert
Thank you !

@justaugustus justaugustus mentioned this pull request Jul 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justaugustus justaugustus justaugustus requested changes

@hasheddan hasheddan Awaiting requested review from hasheddan

@hoegaarden hoegaarden Awaiting requested review from hoegaarden

Assignees

@justaugustus justaugustus

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

[Flaky Test] build-packages-debs (ci-release-build-packages-debs)
4 participants
@saschagrunert @justaugustus @k8s-ci-robot @hwdef