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

chore(ci): fix storage use, go version and lint job #3829

Merged
merged 5 commits into from
Aug 4, 2023

Conversation

mloiseleur
Copy link
Contributor

@mloiseleur mloiseleur commented Jul 31, 2023

Description

There are currently multiple issues in the CI system:

  1. Go version used for build All Images is still go 1.19, it was missed in Upgrade ExternalDNS to go 1.20 #3673
  2. pull-external-dns-lint is launched on each PR and cannot succeed
  3. Build all images is taking too much space on disk and fails on many PRs

This PR fixes this.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 31, 2023
@mloiseleur

This comment was marked as outdated.

1 similar comment
@mloiseleur
Copy link
Contributor Author

/test pull-external-dns-lint

@mloiseleur

This comment was marked as outdated.

@k8s-ci-robot
Copy link
Contributor

@mloiseleur: The following commands are available to trigger required jobs:

  • /test pull-external-dns-lint
  • /test pull-external-dns-unit-test

Use /test all to run all jobs.

In response to this:

/test ?

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.

@mloiseleur

This comment was marked as outdated.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 1, 2023
@mloiseleur mloiseleur changed the title chore(ci): use go 1.20 on all gh workflow chore(ci): fix storage use, go version and lint job Aug 1, 2023
@mloiseleur
Copy link
Contributor Author

mloiseleur commented Aug 1, 2023

@Raffo @szuecs @johngmyers In this PR, I found a way to fix:

  1. pull-external-dns-lint: by forcing download of golangci-lint within the Makefuile
  2. disk usage of Build all images: by cleaning disk between each steps
    • On a GoLang project, I am unsure if it's relevant to build those three arch on each PRs.
  3. go version, which was different from the others Job

Also, I'm not sure to understand why there is two lint and if we should remove one.

@johngmyers
Copy link
Contributor

The prow jobs have the advantage of retesting against the result of a proposed merge. They can also be re-run by people who aren't approvers. GitHub Actions have the advantage of being able to run in the author's fork prior to opening a PR.

The downloading of golangci-lint in the Makefile seems reasonable. I was thinking of copying another project's script, but I don't think that makes much difference.

I'm not so sure about the build-all-images change.

@johngmyers
Copy link
Contributor

Is there any particular reason we need an alpine base image? Can we switch to ko and chainguard?

@johngmyers
Copy link
Contributor

Okay, the build.image/multiarch target iterates over each of the architectures anyway, so that change seems fairly innocuous.
/ok-to-test
/lgtm
/assign @njuettner

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 1, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2023
@johngmyers
Copy link
Contributor

Opened a WIP PR to switch the build system to ko in #3835

@mloiseleur
Copy link
Contributor Author

@johngmyers Interesting, thanks. ko build seems far better.

@Raffo
Copy link
Contributor

Raffo commented Aug 3, 2023

Is there any particular reason we need an alpine base image?

@johngmyers there's #3396 open to address that. The reason is mostly historical: distroless didn't exist when this project was created and kubernetes didn't have kubectl debug and thus alpine was used, to have something with a package manager (although we run as non root). I think it is reasonable now to change to something else. I would recommend google's distroless as it is what other base images are using.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2023
@mloiseleur
Copy link
Contributor Author

mloiseleur commented Aug 4, 2023

@Raffo @johngmyers In order to unblock current CI, I removed build on armv7. At least, FTM, all tests can pass.

I would be happy to move ko.build once #3835 is ready !

@Raffo
Copy link
Contributor

Raffo commented Aug 4, 2023

This is good to go to make builds work, we can then change things with ko or whatever in another PR.

@Raffo
Copy link
Contributor

Raffo commented Aug 4, 2023

/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 Aug 4, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur, Raffo

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2023
@k8s-ci-robot k8s-ci-robot merged commit 952dc5d into kubernetes-sigs:master Aug 4, 2023
14 checks passed
@mloiseleur mloiseleur deleted the fix/build-workflow branch August 4, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants