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

Jump to bottom

Update release-tools to support building on Mac OS X #267

Merged
merged 11 commits into from
May 18, 2020
Merged

Update release-tools to support building on Mac OS X #267

merged 11 commits into from
May 18, 2020

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented May 8, 2020 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Building / running on tests on Mac OS X does not work. This change updates the release-tools to the kubernetes-csi/csi-release-tools@9084fec which already has support for native building/compiling.

Does this PR introduce a user-facing change?:

Update release-tools to 9084fec to support native building

pohly and others added 10 commits March 4, 2020 11:39
@pohly
build for multiple platforms only in CI, add s390x
3863a0f 
Developers should not be forced to build for all platforms by
default. We also don't want to copy-and-paste the go invocation for
each new platform.

To address both, the target platform(s) are now configurable via
BUILD_PLATFORMS and additional platforms are only enabled in the Prow
CI.

For now this serves as a test that the source actually compiles for
multiple platforms. Building images for different target platforms is a
different problem.
@k8s-ci-robot
Merge pull request kubernetes-csi#76 from pohly/build-targets
4cf843f 
build for multiple platforms only in CI, add s390x
@ggriffiths
Update snapshotter to version 2.0.1
7edc146 
Signed-off-by: Grant Griffiths <[email protected]>
@k8s-ci-robot
Merge pull request kubernetes-csi#77 from ggriffiths/snapshotter201_u…
152396e 
…pdate

Update snapshotter to version 2.0.1
@msau42
update release tools instructions
ea1f94a
@k8s-ci-robot
Merge pull request kubernetes-csi#80 from msau42/add-release-process
c1bdf5b 
update release tools instructions
@ggriffiths
Support local snapshot RBAC for pull jobs
d8c76fe 
Signed-off-by: Grant Griffiths <[email protected]>
@k8s-ci-robot
Merge pull request kubernetes-csi#78 from ggriffiths/fix_csi_snapshot…
0fcc3b1 
…ter_rbac_version_set

Fix csi-snapshotter RBAC yaml version
@msau42
Update patch release notes generation command
6f2322e
@k8s-ci-robot
Merge pull request kubernetes-csi#81 from msau42/add-release-process
9084fec 
Update patch release notes generation command
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 8, 2020
@k8s-ci-robot k8s-ci-robot requested review from msau42 and saad-ali May 8, 2020 22:21
@k8s-ci-robot
Copy link
Contributor

Hi @timoreimann. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 8, 2020
@timoreimann
Copy link
Contributor Author

/assign @pohly

since we talked about this one a couple of months ago on Slack.

@pohly
Copy link
Contributor

pohly commented May 11, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2020
@timoreimann
Copy link
Contributor Author

All tests seem to pass, and yet the build is red. I can see

make: Target 'test' not remade because of errors.
ERROR: 'make test' failed
WARNING: 'make test' failed, proceeding anyway

being logged.

I wonder if the fact that I modified the release tooling is the problem? This comment hints at that. If that's true, how'd I go about making the necessary change?

@pohly
Copy link
Contributor

pohly commented May 11, 2020

The make test-subtree failure is legitimate. This change must be merged into https://github.com/kubernetes-csi/csi-release-tools/ first, then the updated csi-release-tools needs to be merged into this (and other repos, as needed) via git subtree (https://github.com/kubernetes-csi/csi-release-tools/#sharing-and-updating).

@timoreimann
Update release-tools to commit 9084fec
d73d0cd 
This adds support for building csi-test binaries for the local platform
(as opposed to be being hard-wired to Linux previously).

Merge commit '9084fecb84ddb47af8360d9ebe4f48a6b7a65c1d' into support-building-on-mac-os
@timoreimann timoreimann force-pushed the support-building-on-mac-os branch from 0a37399 to d73d0cd Compare May 17, 2020 08:56
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 17, 2020
@timoreimann timoreimann changed the title Support building on Mac OS X Update release-tools to support building on Mac OS X May 17, 2020
@timoreimann
Copy link
Contributor Author

@pohly turned out kubernetes-csi/csi-release-tools already had support for building natively, so all it takes for us is to bump csi-test's subtree reference to the latest master. I updated the PR accordingly, please take a look again whenever you have a moment.

@timoreimann
Copy link
Contributor Author

Just to be clear because Github makes it look like I pushed more changes than I actually did: for this PR I invoked

git subtree pull --prefix=release-tools https://github.com/kubernetes-csi/csi-release-tools.git master

and pushed a single commit. I think Github's support for displaying subtree-based changes is still behind its capabilities to do the same in submodules cases.

@pohly
Copy link
Contributor

pohly commented May 18, 2020

turned out kubernetes-csi/csi-release-tools already had support for building natively

Would be nice if I had remembered that I already changed that, right? 😝

ETOOMUCHGOINGON

I think Github's support for displaying subtree-based changes is still behind its capabilities to do the same in submodules cases.

This has confused others in the past, too. The advantage of subtree is that it is easy to do a test PR in a component using csi-release-tools and then submit that same commit against csi-release-tools itself. Might be worthwhile to reconsider the tool choice, though.

pohly
pohly approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, timoreimann

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 May 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 661a3cb into kubernetes-csi:master May 18, 2020
@timoreimann timoreimann deleted the support-building-on-mac-os branch July 24, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

@msau42 msau42 Awaiting requested review from msau42

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@pohly pohly

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timoreimann @k8s-ci-robot @pohly @ggriffiths @msau42