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.

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

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

Merged
merged 3 commits into from
May 12, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7dd480e
Make some cleanup in kep prr template and building scripts
rikatz Apr 18, 2021
324b0c6
Add comment about PRR approver list
rikatz Apr 21, 2021
cff76b9
Add again windows and darwin into platforms
rikatz Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions hack/verify-build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,7 @@ set -o pipefail

PLATFORMS=(
linux/amd64
linux/386
linux/arm
linux/arm64
linux/ppc64le
linux/s390x
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!

)

Expand Down
11 changes: 8 additions & 3 deletions keps/NNNN-kep-template/kep.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@ reviewers:
approvers:
- TBD
- "@oscar.doe"
prr-approvers:
- TBD
- "@bob.doe"

##### WARNING !!! ######
# prr-approvers has been moved to its own location
# You should create your own in keps/prod-readiness
# Please make a copy of keps/prod-readiness/template/nnnn.yaml
# to keps/prod-readiness/sig-xxxxx/00000.yaml (replace with kep number)
#prr-approvers:

palnabarun marked this conversation as resolved.
Show resolved Hide resolved
see-also:
- "/keps/sig-aaa/1234-we-heard-you-like-keps"
- "/keps/sig-bbb/2345-everyone-gets-a-kep"
Expand Down
6 changes: 6 additions & 0 deletions keps/prod-readiness/template/nnnn.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# The KEP must have an approver from the
# "prod-readiness-approvers" group
# of http://git.k8s.io/enhancements/OWNERS_ALIASES
kep-number: 0000
alpha:
approver: ""