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

Fix create, push and watch commands on windows #672

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Fix create, push and watch commands on windows #672

merged 1 commit into from
Sep 4, 2018

Conversation

dgolovin
Copy link
Contributor

Fix #641 and #635

surajnarwade
surajnarwade suggested changes Aug 28, 2018
View reviewed changes
pkg/component/component.go Outdated
@@ -131,6 +132,25 @@ func CreateFromPath(client *occlient.Client, name string, componentImageType str
return nil
}

// Reads file path form URL file:///C:/path/to/file to C:\path\to\file
func ReadFilePath(u *url.URL) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

this functions ReadFilePath and GenFilePath should go in pkg/util/util.go

@surajnarwade
Copy link
Contributor

other than one comment, everything looks good to me, worked on my linux machine
Thanks @dgolovin :)

@dgolovin
Copy link
Contributor Author

@surajnarwade thanks for the review. I've moved those methods to util package.

@dgolovin
Copy link
Contributor Author

@kadel could you review it or assign someone?

@surajnarwade
Copy link
Contributor

cc @anmolbabu @mik-dass @syamgk please review this PR

@mik-dass
Copy link
Contributor

@dgolovin @surajnarwade I will test this on a windows machine

cdrage
cdrage suggested changes Aug 30, 2018
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Code LGTM. I have a few comments however. Works on Linux 👍 Need to test on Windows.

pkg/occlient/occlient.go Outdated
@@ -1647,7 +1647,7 @@ func tar(tw *taro.Writer, fileName string, destFile string) error {
}
splitFileName := strings.Split(fileName, destFile)[1]

hdr.Name = destFile + splitFileName
hdr.Name = destFile + strings.Replace(splitFileName, "\\", "/", -1)
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding a small comment that this is intended for Windows / Mac OS / Linux support?

pkg/util/util.go Outdated
@@ -59,3 +62,22 @@ func NamespaceOpenShiftObject(componentName string, applicationName string) (str
// Return the hyphenated namespaced name
return fmt.Sprintf("%s-%s", componentName, applicationName), nil
}

// Reads file path form URL file:///C:/path/to/file to C:\path\to\file
func ReadFilePath(u *url.URL) string {
Copy link
Member

Choose a reason for hiding this comment

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

We should add tests for this.. but we can ignore this for now just so we can fix the commands on Windows for the time being. Can you add a TODO: Write tests comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll look if I can add tests, to make PR checks back to green, if I fail, then adding TODO is my backup plan.

@anmolbabu
Copy link
Contributor

anmolbabu commented Aug 30, 2018 •
edited

@dgolovin does this PR fix #639 also ?
Just asking because I see watch and windows in the PR title 😀

@dgolovin
Copy link
Contributor Author

@anmolbabu I think #639 is a different problem and this fix is not going to help.

@dgolovin
Copy link
Contributor Author

dgolovin commented Aug 31, 2018 •
edited

@cdrage I've added tests and comment, why replacing '\' to '/' is required.

@cdrage
Copy link
Member

cdrage commented Aug 31, 2018

This LGTM!

@dgolovin dgolovin force-pushed the odo-push-watch-windows branch 3 times, most recently from 59dc855 to a90d8b7 Compare August 31, 2018 19:23
@dgolovin
Copy link
Contributor Author

I've squashed all commits in one and fixed some typos in test messages.
@cdrage @anmolbabu @mik-dass @surajnarwade I have no rights to push, could you release it to the master branch?

mik-dass
mik-dass reviewed Sep 4, 2018
View reviewed changes
pkg/util/util.go Outdated

// Converts file path on windows to /C:/path/to/file to work in URL
func GenFileUrl(location string, os string) string {
var urlPath string = location
Copy link
Contributor

@mik-dass mik-dass Sep 4, 2018 •
edited

Choose a reason for hiding this comment

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

I think string is redundant here it can be simplified to var urlPath = location or urlPath:=location

mik-dass
mik-dass reviewed Sep 4, 2018
View reviewed changes
pkg/util/util.go Outdated

// Reads file path form URL file:///C:/path/to/file to C:\path\to\file
func ReadFilePath(u *url.URL, os string) string {
var location string = u.Path
Copy link
Contributor

@mik-dass mik-dass Sep 4, 2018 •
edited

Choose a reason for hiding this comment

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

same as above

mik-dass
mik-dass reviewed Sep 4, 2018
View reviewed changes
pkg/util/util.go Outdated
var location string = u.Path
if os == WIN {
location = strings.Replace(u.Path, "/", "\\", -1)
location = location[1:len(location)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified to location = location[1:]

@dgolovin
Copy link
Contributor Author

dgolovin commented Sep 4, 2018

@mik-dass fixed. Thanks!

mik-dass
mik-dass approved these changes Sep 4, 2018
View reviewed changes
Copy link
Contributor

@mik-dass mik-dass left a comment •
edited

Choose a reason for hiding this comment

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

Code LGTM and I build and ran the PR on windows and it works 👍

@cdrage
Copy link
Member

cdrage commented Sep 4, 2018

Unfortunately this needs a re-based as we merged in some pretty big PRs. The code LGTM however! So after re-base, feel free to merge.

@dgolovin
Fix create, push, watch commands on windows
c73cd86 
Fix adds coversion for windows paths to urls and back
@dgolovin
Copy link
Contributor Author

dgolovin commented Sep 4, 2018

@cdrage rebased on top of master.

@mik-dass mik-dass merged commit a7076a6 into redhat-developer:master Sep 4, 2018
@dgolovin dgolovin mentioned this pull request Sep 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmolbabu anmolbabu anmolbabu approved these changes

@cdrage cdrage cdrage approved these changes

@mik-dass mik-dass mik-dass approved these changes

@surajnarwade surajnarwade surajnarwade approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dgolovin @surajnarwade @mik-dass @anmolbabu @cdrage