feat(util): Allows building for darwin targets #133

thomastaylor312 · 2021-01-28T18:57:23Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is a simple change that allows the registrar to be built for MacOS. All of the unix package being used works for Mac as well. The reason for enabling this is completely selfish I'll admit. We are adding CSI Support to the Krustlet project, which supports Mac as well. As most CSI implementations don't support registering themselves with the Kubelet directly, we wanted to unblock any users to need to register their plugins with a Krustlet Kubelet. This change shouldn't affect normal container-based usage of the registrar

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-01-28T18:57:31Z

Welcome @thomastaylor312!

It looks like this is your first PR to kubernetes-csi/node-driver-registrar 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/node-driver-registrar has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-01-28T18:57:31Z

Hi @thomastaylor312. 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.

7bc70e52 Merge pull request kubernetes-csi#129 from pohly/squash-documentation e0b02e72 README.md: document usage of --squash 316cb957 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate 26e2ab10 fix: default boilerplate path 1add8c18 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag 3e811d6c prow.sh: fix "on-master" prow jobs git-subtree-dir: release-tools git-subtree-split: 7bc70e5264a5ce5f47780bdbc6c7b7f4e79243fa

msau42 · 2021-03-31T01:20:53Z

/ok-to-test

msau42 · 2021-03-31T01:22:28Z

@pohly do you know if we can add this to our build job to make sure this compiles? Better yet, if we can release images with it?

pohly · 2021-03-31T08:04:30Z

@pohly do you know if we can add this to our build job to make sure this compiles?

We only do build and unit tests for the default Go platform on which we run (linux amd64), so this would be a rather bigger change but ...

Better yet, if we can release images with it?

... this should be as simple as adding "darwin linux" to

node-driver-registrar/release-tools/prow.sh

Line 80 in d79871c

    
           configvar CSI_PROW_BUILD_PLATFORMS "linux amd64; windows amd64 .exe; linux ppc64le -ppc64le; linux s390x -s390x; linux arm64 -arm64" "Go target platforms (= GOOS + GOARCH) and file suffix of the resulting binaries"

Well, "should". I just tried it with make push-multiarch-csi-node-driver-registrar BUILD_PLATFORMS="linux amd64;windows amd64 .exe;darwin amd64" PULL_BASE_REF=master REGISTRY_NAME=pohly and "docker buildx" fails for Darwin. The code itself compiled fine.

+ docker buildx build --push --tag pohly/csi-node-driver-registrar:amd64-darwin-canary --platform=darwin/amd64 --file --build-arg binary=./bin/csi-node-driver-registrar --label revision=v2.1.0-1-g083d5ab9 .
"docker buildx build" requires exactly 1 argument.
See 'docker buildx build --help'.

Usage:  docker buildx build [OPTIONS] PATH | URL | -

The same command worked for --platform=linux/amd64 and --platform=windows/amd64.

This might be an issue with my local setup. https://docs.docker.com/buildx/working-with-buildx/#build-multi-platform-images explicitly mentions darwin/amd64.

fejta-bot · 2021-06-29T08:06:24Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

thomastaylor312 · 2021-06-29T16:04:11Z

/remove-lifecycle stale

mauriciopoppe · 2021-08-04T18:13:49Z

Sorry for creating the need for a rebase, I think that the first line should also be changed to //go:build !windows, I think it was added by my editor on save

thomastaylor312 · 2021-08-04T18:27:32Z

Sweet, I'll fix that real quick

This is a simple change that allows the registrar to be built for MacOS. All of the `unix` package being used works for Mac as well. The reason for enabling this is completely selfish I'll admit. We are adding [CSI Support](krustlet/krustlet#462) to the Krustlet project, which supports Mac as well. As most CSI implementations don't support registering themselves with the Kubelet directly, we wanted to unblock any users to need to register their plugins with a Krustlet Kubelet. This change shouldn't affect normal container-based usage of the registrar

msau42 · 2021-08-11T00:24:33Z

pkg/util/util_nix.go

@@ -1,5 +1,5 @@
-//go:build linux
-// +build linux
+//go:build !windows


This is the first time I've seen the "nix" suffix being used. Is there a general convention that the Kubernetes project is using to support darwin?

@thomastaylor312: can you explain where the _nix.go filename comes from? Is that being used elsewhere already?

We can just call it unix if you prefer. I was just trying to indicate that this file wasn't for just linux, but any *nix system. I don't have any sort of a strong preference. Also no skin off of my teeth if you don't want to support darwin targets. This was just a proposal of something to do and can be shot down. As you can see, there was no change to code, so the restriction to linux build targets only was artificial and not for any specific code reason insofar as I can tell

I (like @msau42 I guess) was just wondering. Personally I prefer "unix" over "nix". The latter might not be immediately recognizable as a placeholder for "unix or linux".

Kubernetes releases darwin/amd64 and darwin/arm64 client binaries, but not node binaries. I'm fine with merging this PR, but I would not actually build container images for Darwin.

Yeah, no need to build a container image here. Let me rename the file real quick

Ok, file has been renamed!

k8s-triage-robot · 2021-11-09T01:10:27Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

thomastaylor312 · 2021-11-09T18:16:04Z

/remove-lifecycle stale

k8s-triage-robot · 2022-02-07T18:26:44Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

thomastaylor312 · 2022-02-07T18:27:12Z

/remove-lifecycle stale

k8s-triage-robot · 2022-05-08T18:33:28Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

thomastaylor312 · 2022-05-23T22:23:03Z

/remove-lifecycle stale

Signed-off-by: Taylor Thomas <[email protected]>

pohly

/lgtm

pohly · 2022-05-24T20:16:35Z

/assign @jsafrane

For approval.

jsafrane · 2022-05-27T10:01:47Z

/approve

k8s-ci-robot · 2022-05-27T10:01:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, pohly, thomastaylor312

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:

~~OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thomastaylor312 · 2022-05-27T19:13:31Z

Thanks all!

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 28, 2021

k8s-ci-robot requested review from lpabon and msau42 January 28, 2021 18:57

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 Jan 28, 2021

thomastaylor312 mentioned this pull request Jan 28, 2021

PVC mount support krustlet/krustlet#462

Merged

pohly mentioned this pull request Feb 3, 2021

master: update release-tools #134

Merged

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 Mar 31, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 29, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2021

thomastaylor312 force-pushed the feat/support_mac branch from 083d5ab to 24a855e Compare August 3, 2021 16:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2021

thomastaylor312 force-pushed the feat/support_mac branch from 24a855e to 9d1ccf4 Compare August 4, 2021 18:28

msau42 reviewed Aug 11, 2021

View reviewed changes

mauriciopoppe mentioned this pull request Aug 25, 2021

master: update release-tools kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#812

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 9, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 9, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 7, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 7, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 23, 2022

Renames file

c9666a6

Signed-off-by: Taylor Thomas <[email protected]>

pohly approved these changes May 24, 2022

View reviewed changes

k8s-ci-robot assigned pohly May 24, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2022

k8s-ci-robot assigned jsafrane May 24, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2022

k8s-ci-robot merged commit 5aa7c89 into kubernetes-csi:master May 27, 2022

thomastaylor312 deleted the feat/support_mac branch May 27, 2022 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(util): Allows building for darwin targets #133

feat(util): Allows building for darwin targets #133

thomastaylor312 commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

msau42 commented Mar 31, 2021

msau42 commented Mar 31, 2021

pohly commented Mar 31, 2021

fejta-bot commented Jun 29, 2021

thomastaylor312 commented Jun 29, 2021

mauriciopoppe commented Aug 4, 2021 •

edited

Loading

thomastaylor312 commented Aug 4, 2021

msau42 Aug 11, 2021

pohly May 24, 2022

thomastaylor312 May 24, 2022

pohly May 24, 2022

thomastaylor312 May 24, 2022

thomastaylor312 May 24, 2022

k8s-triage-robot commented Nov 9, 2021

thomastaylor312 commented Nov 9, 2021

k8s-triage-robot commented Feb 7, 2022

thomastaylor312 commented Feb 7, 2022

k8s-triage-robot commented May 8, 2022

thomastaylor312 commented May 23, 2022

pohly left a comment

pohly commented May 24, 2022

jsafrane commented May 27, 2022

k8s-ci-robot commented May 27, 2022

thomastaylor312 commented May 27, 2022

feat(util): Allows building for darwin targets #133

feat(util): Allows building for darwin targets #133

Conversation

thomastaylor312 commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

msau42 commented Mar 31, 2021

msau42 commented Mar 31, 2021

pohly commented Mar 31, 2021

fejta-bot commented Jun 29, 2021

thomastaylor312 commented Jun 29, 2021

mauriciopoppe commented Aug 4, 2021 • edited Loading

thomastaylor312 commented Aug 4, 2021

msau42 Aug 11, 2021

Choose a reason for hiding this comment

pohly May 24, 2022

Choose a reason for hiding this comment

thomastaylor312 May 24, 2022

Choose a reason for hiding this comment

pohly May 24, 2022

Choose a reason for hiding this comment

thomastaylor312 May 24, 2022

Choose a reason for hiding this comment

thomastaylor312 May 24, 2022

Choose a reason for hiding this comment

k8s-triage-robot commented Nov 9, 2021

thomastaylor312 commented Nov 9, 2021

k8s-triage-robot commented Feb 7, 2022

thomastaylor312 commented Feb 7, 2022

k8s-triage-robot commented May 8, 2022

thomastaylor312 commented May 23, 2022

pohly left a comment

Choose a reason for hiding this comment

pohly commented May 24, 2022

jsafrane commented May 27, 2022

k8s-ci-robot commented May 27, 2022

thomastaylor312 commented May 27, 2022

mauriciopoppe commented Aug 4, 2021 •

edited

Loading