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

🐛 fix windows filepath compatibility #179

Merged
merged 7 commits into from
Apr 29, 2019

Conversation

alexeldeib
Copy link
Contributor

@alexeldeib alexeldeib commented Apr 8, 2019

Fixes #152, enables Kubebuilder Windows releases.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 8, 2019
@alexeldeib alexeldeib changed the title 🐛windows filepath compatibility 🐛 fix windows filepath compatibility Apr 8, 2019
@DirectXMan12
Copy link
Contributor

It seems like you missed a few (there's a couple of spots that're still path.Join)

@alexeldeib
Copy link
Contributor Author

alexeldeib commented Apr 8, 2019

@DirectXMan12 I took the slimmest possible cut to make this functional. Is it safe to assume we'd like to change all of these to filepath and not path? I'll align the rest if so.

@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 Apr 8, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2019
@alexeldeib
Copy link
Contributor Author

alexeldeib commented Apr 9, 2019

Per kubernetes-sigs/kubebuilder#661 (comment), I'm going to revert to the initial cut using fewer filepath calls. I don't see a point switching back and forth on filepath separators except where it's necessary.

Maybe I should really be aligning the strings.HasPrefix() calls + paths at the same time to make it a non-issue? It's the mismatch between Go imports, HasPrefix, and Windows that is causing the problem anyway.

Meh, a good number of these are actually filepaths.

@alexeldeib
Copy link
Contributor Author

@DirectXMan12 let me know the preferred route on this and I can implement it. Sorry to nag you, would love to get full releases of kubebuilder enabled for windows 😄

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 15, 2019
@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 25, 2019
@DirectXMan12
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2019
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

setAPIsPath still has some weirdness, no? I think you need to convert to a OS path when calling stat, but leave as a normal path otherwise?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2019
@alexeldeib
Copy link
Contributor Author

Nice catch, fixed on the os.Stat.

@DirectXMan12
Copy link
Contributor

/lgtm
/approve

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 29, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, DirectXMan12

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

@DirectXMan12
Copy link
Contributor

thanks!

@k8s-ci-robot k8s-ci-robot merged commit 24e981c into kubernetes-sigs:master Apr 29, 2019
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. 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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Util funcs do not correctly handle windows paths
3 participants