Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

More hermetic, unified builds #1022

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

adrianludwin
Copy link
Contributor

@adrianludwin adrianludwin commented Aug 17, 2020

This commit makes two significant changes:

  • Includes the kustomize tool so that we don't need to either fetch it
    from the internet or rely on one being installed locally.
  • Replaces portions of the Cloud Build process with the equivalent steps
    from the Makefile so they can't get out of sync.

As a side effect, this required a bit of refactoring in the makefile,
and I've moved some lines around to be more closely located with the
relevant recipes.

I tried removing kubebuilder (I thought we just needed controller-gen)
but it contains elements required by Ginkgo. Since it's rather large
(>50MB) and doesn't affect the correctness of the binaries as they're
built (only tested), I decided not to check it in. I did upgrade the
version we use from a 2.0 alpha to the latest release (as of Aug 2020).

Finally, this includes a small docs change that was mistakenly left out
of an earlier PR.

Tested: made all targets locally; confirmed kubectl hns version gave
the expected result; pushed a test tag to my repo and used the
HNC_RELEASE_REPO_OWNER flag to build a fake release from that tag.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 17, 2020
@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 17, 2020
@adrianludwin adrianludwin force-pushed the kustomize branch 3 times, most recently from 9eb3e1c to 688d02a Compare August 17, 2020 21:46
@adrianludwin adrianludwin changed the title Hermetic, unified builds More hermetic, unified builds Aug 17, 2020
@adrianludwin
Copy link
Contributor Author

/assign @rjbez17
/assign @yiqigao217
/cc @GinnyJI

incubator/hnc/Makefile Show resolved Hide resolved
incubator/hnc/Makefile Show resolved Hide resolved
incubator/hnc/Makefile Show resolved Hide resolved
incubator/hnc/cloudbuild.yaml Show resolved Hide resolved
@yiqigao217
Copy link
Contributor

Thanks for the explanations! @adrianludwin can you help me resolve the comments (somehow I don't have that option).

/lgtm
/hold
/assign @rjbez17

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 18, 2020
@adrianludwin
Copy link
Contributor Author

/wip

This conflicts with #1032 so I need to revise

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2020
@k8s-ci-robot
Copy link
Contributor

@adrianludwin: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-mtb-test be67541 link /test pull-mtb-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@k8s-ci-robot k8s-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 28, 2020
@adrianludwin adrianludwin force-pushed the kustomize branch 2 times, most recently from ff297a0 to 4fa7fbe Compare August 28, 2020 17:54
@adrianludwin
Copy link
Contributor Author

/hold cancel

Updated to include multiplatform kubectl plugins. Tested by releasing to https://github.com/adrianludwin/multi-tenancy/releases/tag/hnc-v0.6.0-test1.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2020
@adrianludwin
Copy link
Contributor Author

@yiqigao217 Can you please re-review? Note that the hold is gone.
@rjbez17 Are you interested in reviewing this?

Thanks, A

Copy link
Contributor

@yiqigao217 yiqigao217 left a comment

Choose a reason for hiding this comment

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

lgtm just 1 nit

incubator/hnc/hack/README.md Outdated Show resolved Hide resolved
@adrianludwin adrianludwin force-pushed the kustomize branch 3 times, most recently from c60e42d to 219e18e Compare August 28, 2020 20:07
This commit makes two significant changes:

* Includes the kustomize tool so that we don't need to either fetch it
from the internet or rely on one being installed locally.
* Replaces portions of the Cloud Build process with the equivalent steps
from the Makefile so they can't get out of sync.

As a side effect, this required a bit of refactoring in the makefile,
and I've moved some lines around to be more closely located with the
relevant recipes.

I tried removing `kubebuilder` (I thought we just needed controller-gen)
but it contains elements required by Ginkgo. Since it's rather large
(>50MB) and doesn't affect the correctness of the binaries as they're
built (only tested), I decided not to check it in. I did upgrade the
version we use from a 2.0 alpha to the latest release (as of Aug 2020).

Finally, this includes a small docs change that was mistakenly left out
of an earlier PR.

Tested: made all targets locally; confirmed `kubectl hns version` gave
the expected result; pushed a test tag to my repo and used the
HNC_RELEASE_REPO_OWNER flag to build a fake release from that tag.
Copy link
Contributor

@yiqigao217 yiqigao217 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 23e6e24 into kubernetes-retired:master Aug 28, 2020
@adrianludwin adrianludwin added this to the hnc-v0.6 milestone Sep 23, 2020
@adrianludwin adrianludwin deleted the kustomize branch October 5, 2020 15:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants