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

Make some cleanup in KEP PRR template and building scripts #2636

Merged
merged 3 commits into from
May 12, 2021

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Apr 18, 2021

This PR:

  • Cleans the verify-build.sh to build only for amd64 (as we run the tests in amd64 only, right?) cutting a bit the tests time
  • Make some statement about creating the PRR file on the right place, removing it from kep.yaml

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 18, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 18, 2021
windows/amd64
windows/386
freebsd/amd64
darwin/amd64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can keep darwin and windows/amd64, but I'm really wondering if we need the others.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed the same thing today, 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep! So for example, currently only with linux/amd64:

time ./hack/verify-build.sh 
Building project for linux/amd64

real    0m6,042s
user    0m16,617s
sys     0m2,836s

While adding windows and darwin:

time ./hack/verify-build.sh 
Building project for linux/amd64
Building project for windows/amd64
Building project for darwin/amd64

real    0m20,079s
user    1m28,200s
sys     0m8,459s

And so goes this. So I can say this PR improves the KEP CI performance by 900% :P

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep darwin/amd64, linux/amd64 and possibly linux/arm64. (don't know if anyone uses kepctl on a Raspberry Pi or a similar device 😅)

Wondering if we should add darwin/arm64 as well? Many folks are moving to ARM Macs, although I don't have any metrics to prove the same.

Copy link
Member

Choose a reason for hiding this comment

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

As an addition, I am feeling like we should expose an env var which let's users the flexibility with specifying what they want to build for. Maybe $PLATFORM itself?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep darwin/amd64, linux/amd64 and possibly linux/arm64. (don't know if anyone uses kepctl on a Raspberry Pi or a similar device 😅)

Raspis are armhf or 32-bit ARM so this is N/A :) Most arm64 devices are servers, we probably can skip it. But we should probably keep Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't add darwin/arm64 as it wasn't there and no one actually missed it :P

But anyway keeping darwin/amd64, linux/amd64 and windows/amd64 only is already a win.

About adding the variable, I'm not against it, just wondering if we want this to be a multi arg variable, or if a single one (like BUILD_PLATFORM=$1) and verify if this is not empty, use it otherwise use the pre-populated PLATFORMS.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Raspis are armhf or 32-bit ARM so this is N/A :)

Ah! E_TOO_MANY_ARCHS 😅

I wouldn't add darwin/arm64 as it wasn't there and no one actually missed it :P

LOL. Makes sense! If someone wants to, we can add later.

About adding the variable, I'm not against it, just wondering if we want this to be a multi arg variable, or if a single one (like BUILD_PLATFORM=$1) and verify if this is not empty, use it otherwise use the pre-populated PLATFORMS.

The best option is a multi-arg variable, then a single variable with comma-separated values. I'm fine with either option. Also, pondering more over it, I'm thinking we can lazily add this feature if someone needs it in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me :D

Let me push with darwin/amd64 and windows/amd64 and let me know if you want me to change anything else here :D

Copy link
Member

Choose a reason for hiding this comment

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

All okay from my side. Thank you for the changes!

keps/prod-readiness/template/nnnn.yaml Outdated Show resolved Hide resolved
windows/amd64
windows/386
freebsd/amd64
darwin/amd64
Copy link
Member

Choose a reason for hiding this comment

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

I noticed the same thing today, 👍

Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
keps/NNNN-kep-template/kep.yaml Show resolved Hide resolved
windows/amd64
windows/386
freebsd/amd64
darwin/amd64
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep darwin/amd64, linux/amd64 and possibly linux/arm64. (don't know if anyone uses kepctl on a Raspberry Pi or a similar device 😅)

Wondering if we should add darwin/arm64 as well? Many folks are moving to ARM Macs, although I don't have any metrics to prove the same.

windows/amd64
windows/386
freebsd/amd64
darwin/amd64
Copy link
Member

Choose a reason for hiding this comment

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

As an addition, I am feeling like we should expose an env var which let's users the flexibility with specifying what they want to build for. Maybe $PLATFORM itself?

Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
@rikatz
Copy link
Contributor Author

rikatz commented Apr 29, 2021

/label tide/merge-method-squash

because I'm too lazy to rebase :P

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 29, 2021
@palnabarun
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2021
@palnabarun
Copy link
Member

/assign @jeremyrickard for approval

@k8s-ci-robot
Copy link
Contributor

@palnabarun: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @jeremyrickard for approval

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.

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.

Thank you!
/lgtm
/approve

@justaugustus
Copy link
Member

/assign @johnbelamaric

@johnbelamaric
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2021
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, johnbelamaric, justaugustus, rikatz

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 171f02c into kubernetes:master May 12, 2021
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. area/enhancements Issues or PRs related to the Enhancements subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants