add URL support #64

concaf · 2017-03-09T20:45:22Z

surajssd · 2017-03-13T11:02:20Z

pkg/cmd/convert.go

 )

 var (
 	convertExample = `  # Converts file
  opencompose convert -f opencompose.yaml`
 )

+const (
+	retryAttempts = 3


shouldn't this be all caps?

not really, C (and MACROS) ages are gone :)

I know it's not a C age, it's just that it becomes easier to understand just by looking at code that it's a constant and no need to find it's declaration in immediate scope.

@surajssd well, the block is called const that should help you. But seriously in Golang case matters - if you change it to all uppercase you export that constant outside of the package

@surajssd also, some context from k8s - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/resource/visitor.go#L44-L47

surajssd · 2017-03-13T11:03:59Z

pkg/cmd/convert.go

@@ -62,20 +69,44 @@ func GetValidatedObject(v *viper.Viper, cmd *cobra.Command, out, outerr io.Write
 	}

 	var ocObjects []*object.OpenCompose
+	var data []byte
+	var err error
 	for _, file := range files {


can this be better named as filepath

s/file/filepath ?

@surajssd I didn't edit this part of code in this PR, let's send this a separate one?

I don't think it needs renaming. Especially as it's not changed here. (Btw. Go guidelines actually tell you to use really short names, but to be fair it's no applicable everywhere.)

Using filePath makes more sense over using file because it conveys the meaning of what that variable has, here it has path to the file than file itself.

@surajssd I would say it is really the last think we should be discussing at this point and it is not a subject of this PR.

surajssd · 2017-03-13T11:29:37Z

pkg/cmd/convert.go

+			// Read from the response body, ioutil.ReadAll will return []byte
+			data, err = ioutil.ReadAll(body)
+			if err != nil {
+				return nil, fmt.Errorf("Reading from response body failed")


how about using the wrapping library right from now, for errors?

I think the Golang way is

return nil, fmt.Errorf("reading from response body failed: %s", err)

tnozicka

some nits
exctraction into a function
needs unit tests

tnozicka · 2017-03-13T17:54:22Z

pkg/cmd/convert.go

+		// Check if the passed resource is a URL or not
+		if strings.HasPrefix(file, "http://") || strings.HasPrefix(file, "https://") {
+
+			// Validate URL


could you move the logic bellow into a separate function? it will make better separation of concerns and it will be separately testable without creating cobra objects

tnozicka · 2017-03-13T17:57:39Z

pkg/util/util.go

+		}
+
+		// retry if http.Get fails
+		response, err = http.Get(url)


Having := instead of pre-declaring response would look more like Golang.

(in general I declare a variable upfront only if it extends the scope or to avoid shadowing)

tnozicka · 2017-03-14T09:09:51Z

pkg/cmd/convert.go

@@ -62,15 +69,39 @@ func GetValidatedObject(v *viper.Viper, cmd *cobra.Command, out, outerr io.Write
 	}

 	var ocObjects []*object.OpenCompose
+	var data []byte


I think that these two variable should stay declared inside the loop as they were before. Otherwise you are extending the scope and a life time of them which can be error prone

@tnozicka we need to use data outside the if else statement, for passing it to decoder, etc. How do you suggest I do this without declaring this before if else?

@containscafeine you don't use data outside of that for. declare it before the first if inside for

Oh wait, I put it outside the loop, it should be inside the loop out the if, else; sorry got a bit confused :)

tnozicka · 2017-03-14T09:11:01Z

pkg/cmd/convert.go

-			return nil, fmt.Errorf("unable to read file '%s': %s", file, err)
+		// Check if the passed resource is a URL or not
+		if strings.HasPrefix(file, "http://") || strings.HasPrefix(file, "https://") {
+


we can leave out this empty line after if so it looks like the rest of the code

tnozicka · 2017-03-14T09:13:02Z

pkg/util/util.go

+// Wait for <duration> time between each try.
+// This return the response body upon successful fetch
+func FetchURLWithRetries(url string, attempts int, duration time.Duration) (io.ReadCloser, error) {
+


tnozicka · 2017-03-14T09:16:26Z

pkg/util/util.go

+		}
+
+		body = response.Body
+	}


@tnozicka not sure about what do you want me to change here?

@containscafeine the was an empty line missing, now it's ok

concaf · 2017-03-15T14:45:50Z

@tnozicka PTAL, made the changes!

tnozicka · 2017-03-15T14:55:00Z

@containscafeine tests are failing https://travis-ci.org/redhat-developer/opencompose/jobs/211376290
try make format

I'll review it after.

concaf · 2017-03-15T15:32:30Z

@tnozicka oops, sorry about that, fixed :)

tnozicka

@containscafeine I am bit in a rush here because I wanted to make it before going on PTO; I hope it will make sense otherwise it's most probably my fault ;)

tnozicka · 2017-03-15T15:44:52Z

pkg/util/util.go

+		}
+
+		body = response.Body
+	}


@containscafeine the was an empty line missing, now it's ok

tnozicka · 2017-03-15T15:46:55Z

pkg/util/util.go

+	"time"
+)
+
+func ValidateFetchReadURL(urlString string, attempts int) ([]byte, error) {


ValidateFetchReadURL->GetURLData?

makes sense, fixed

tnozicka · 2017-03-15T15:47:43Z

pkg/util/util.go

+	// Validate URL
+	_, err := url.ParseRequestURI(urlString)
+	if err != nil {
+		return nil, fmt.Errorf("%v: invalid URL", urlString)


s/"%v: invalid URL/"invalid URL: %v/

tnozicka · 2017-03-15T15:49:43Z

pkg/util/util.go

+	// Fetch the URL and store the response body
+	body, err := FetchURLWithRetries(urlString, attempts, time.Second)
+	if err != nil {
+		return nil, err


I think you should wrap that error with an error message like:

fmt.Errorf("FetchURLWithRetries failed: %s", err)

tnozicka · 2017-03-15T15:51:51Z

pkg/util/util_test.go

+
+	for _, tt := range tests {
+		_, err := ValidateFetchReadURL(tt.url, testAttempts)
+		if err != nil && tt.succeed {


this won't work for a case where the test should have failed but it succeeded

missed this somehow, fixed

tnozicka · 2017-03-15T15:53:24Z

pkg/util/util_test.go

+	}
+
+	for _, tt := range tests {
+		_, err := ValidateFetchReadURL(tt.url, testAttempts)


We are moving to using Golang subtests. Please use t.Run as in https://github.com/tnozicka/opencompose/blob/a21430158830bff240a8252aa4b609e994907238/pkg/encoding/v1/encoding_test.go#L207

You can find out more on https://blog.golang.org/subtests

fixed, TIL, thanks :)

tnozicka · 2017-03-15T15:56:20Z

pkg/util/util_test.go

+		url     string
+		succeed bool
+	}{
+		"example.com", true,


I'd like to see here more cases or you can maybe cover it well even from the TestValidateFetchReadURL. But I am generally missing a fail case here and an empty string case.

added more cases, fixed!

concaf · 2017-03-16T20:37:25Z

@tnozicka thanks for the review :) I've made the fixes, PTAL.
Will squash once this is LGTM.

tnozicka

@containscafeine thanks for addressing the previous review and adding test. The structure and tests are looking good; there is a small issue with retry logic though

tnozicka · 2017-03-22T13:20:41Z

pkg/util/util.go

+	}
+
+	// Fetch the URL and store the response body
+	body, err := FetchURLWithRetries(urlString, attempts, time.Second)


I would prefer to be more explicit here: s/time.Second/1*time.Second/

tnozicka · 2017-03-22T13:36:47Z

pkg/util/util.go

+		}
+
+		// if the status code is not 200 OK, return an error
+		if response.StatusCode != http.StatusOK {


I would think there are other successful status codes than just 200. In general the whole 2xx family. 203 comes to my mind and I think it would be a valid response returning data. There might be more.

@tnozicka while going through the entire 200 status list, I'm not sure if all them can be treated as OK.
Also, Kubernetes for reference - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/resource/visitor.go#L269

I partially agree with you, but I don't think we should do this for the entire 200 family. Let me know what should we go ahead with this, I'm a bit divided on this.

In out case only 200 is success, maybe also 203, but not sure if it makes sense in this use-case.
I wound't worry about it.

tnozicka · 2017-03-22T13:37:15Z

pkg/util/util.go

+
+		// if the status code is not 200 OK, return an error
+		if response.StatusCode != http.StatusOK {
+			return nil, fmt.Errorf("unable to fetch %v, server returned status code %v", url, response.StatusCode)


If 3 request would hit e.g. 404, 404, 200 this condition would end up with the first one and didn't continue, right? This may be because of cache propagation. I would consider retrying in that case. (citing the 404 description: "The requested resource could not be found but may be available in the future. Subsequent requests by the client are permissible.")

Also here is a resource leak. You are not closing the body. I might be best if you would use defer + readAll in this function and returned just the data.

If 3 request would hit e.g. 404, 404, 200 this condition would end up with the first one and didn't continue, right? This may be because of cache propagation. I would consider retrying in that case. (citing the 404 description: "The requested resource could not be found but may be available in the future. Subsequent requests by the client are permissible.")

Yep, if a 404 is encountered in the first time, no retries are made and opencompose errors out.
The cache propagation case you have mentioned seems like an edge case, should we really take that into consideration in opencompose?

Also here is a resource leak. You are not closing the body. I might be best if you would use defer + readAll in this function and returned just the data.

I'm closing the body in GetURLData, but I've modified FetchURLWithRetries to return the data as []byte now.

RE: error 404
I don't think we should retry on 404.
Even if official description says "may be available in the future" I don't think it makes sense to retry it in couple of seconds. It should display error.

tnozicka · 2017-03-22T13:53:16Z

pkg/util/util_test.go

+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("passing %v, expected to succeed: %v", tt.url, tt.succeed),
+			func(t *testing.T) {


I would prefer to merge this line with the one above. It will save you one \t for all the commands inside it.

tnozicka · 2017-03-22T13:56:00Z

pkg/util/util_test.go

+func TestGetURLData(t *testing.T) {
+	tests := []struct {
+		url     string
+		succeed bool


the convention in other test is to put succeed in the first place in the struct and test - for consistency it would be nice to follow it

tnozicka · 2017-03-22T14:01:33Z

pkg/util/util_test.go

+)
+
+const (
+	testAttempts = 2


any reason not to test with value of retryAttempts since that the hardcoded value for "prod". are the tests unnecessary slower?

no real reason, fixed.

tnozicka · 2017-03-22T14:02:25Z

pkg/util/util_test.go

+	}
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("URL %v, expected output: %v", tt.url, tt.succeed),
+			func(t *testing.T) {


same as above

tnozicka · 2017-03-22T15:05:23Z

Also it would be nice to mention it in README so you get more credit :)

concaf · 2017-03-23T09:37:42Z

@tnozicka added to README, and made the fixes. There are also some go fmt changes in there. PTAL.

kadel · 2017-03-27T12:47:29Z

pkg/util/util.go

+// This returns the data from  the response body []byte upon successful fetch
+// The passed URL is not validated, so validate the URL before passing to this
+// function
+func FetchURLWithRetries(url string, attempts int, duration time.Duration) ([]byte, error) {


Maybe just one more stupid question here.

Do we even need retry for this? 😇

@kadel imo, it's a nice to have, and since the code is already there in this PR, we can keep it.
Anyway, it retries only if getting a valid resource fails, so should be fine, WDYT?

yeh, I was just wondering if there was some reason for it. 👍
its ok

surajssd · 2017-03-27T17:35:33Z

@containscafeine please don't refer commits with pound sign, cause github is referring it to some old issues/PRs!

kadel · 2017-03-28T13:10:50Z

I think this is ready to be merged once you squash commits

concaf · 2017-03-28T14:18:56Z

@kadel cool, squashed :)

kadel · 2017-03-30T06:56:20Z

pkg/util/util.go

+		if err != nil {
+			return nil, fmt.Errorf("reading from response body failed: %s", err)
+		}
+		defer response.Body.Close()


Sorry, didn't noticed that before :-(
Shouldn't be this defer before calling ReadAll (just after http.Get)?

If ReadAll fails, defer function wont be called.

@kadel, true, nice catch :)
Fixed.

defer in wrong place?

concaf · 2017-04-03T07:04:16Z

@kadel rebased :)

kadel

lgtm

not working

kadel · 2017-04-04T12:01:29Z

its not working for me :-(

▶ ./opencompose convert -f https://raw.githubusercontent.com/kadel/opencompose/sprint-demo2/examples/hello-nginx-external.yaml
ERROR: unable to read file 'https://raw.githubusercontent.com/kadel/opencompose/sprint-demo2/examples/hello-nginx-external.yaml': open https://raw.githubusercontent.com/kadel/opencompose/sprint-demo2/examples/hello-nginx-external.yaml: no such file or directory

concaf · 2017-04-04T12:48:16Z

@kadel the STDIN PR broke this :(
Fixed now, PTAL.

Try -
opencompose convert -f https://gist.github.com/containscafeine/61ec907bcacb776b3e520b529ecdd25e/raw/1051b610f80e9a319ecb9be8b621c551f69dd15f/opencompose.yaml

tnozicka

small nit, lgtm otherwise

tnozicka · 2017-04-04T15:34:02Z

pkg/util/util_test.go

+	}
+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("passing %v, expected to succeed: %v", tt.url, tt.succeed), func(t *testing.T) {


This should be just a test name, not a log message.

tnozicka · 2017-04-04T16:00:08Z

pkg/util/util_test.go

+		{false, "https://google.com/giveme404"}, // test for !200 status code
+	}
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("URL %v, expected output: %v", tt.url, tt.succeed), func(t *testing.T) {


test name, not a log message

This commit allows to pass URLs besides local files to the -f/--file switch. This is done by checking if the passed in resource string starts with an http:// or https://, and if it does, the URL validation is done followed by fetching the URL with 3 retry attempts with a gap of 1 second in between. Fix redhat-developer#55

concaf · 2017-04-05T07:50:08Z

@tnozicka fixed, PTAL

tnozicka · 2017-04-05T08:00:37Z

@containscafeine thanks, lgtm.

@kadel PTAL and merge if you see fit

concaf force-pushed the specify_url branch from 5a93873 to 1d6ddca Compare March 9, 2017 21:47

concaf changed the title ~~[WIP] add URL support~~ add URL support Mar 9, 2017

tnozicka self-requested a review March 10, 2017 17:43

surajssd reviewed Mar 13, 2017

View reviewed changes

concaf force-pushed the specify_url branch from 1d6ddca to 98234f7 Compare March 13, 2017 17:02

tnozicka suggested changes Mar 14, 2017

View reviewed changes

tnozicka assigned concaf Mar 14, 2017

tnozicka self-requested a review March 14, 2017 10:35

tnozicka modified the milestones: Sprint 129, v0.1.0 Mar 14, 2017

kadel modified the milestones: Sprint 129, v0.1.0 Mar 14, 2017

concaf force-pushed the specify_url branch from 0e44b39 to 9f03bf2 Compare March 15, 2017 14:44

concaf force-pushed the specify_url branch from 9f03bf2 to 1147db5 Compare March 15, 2017 15:25

tnozicka suggested changes Mar 15, 2017

View reviewed changes

tnozicka added the needs-squash label Mar 15, 2017

concaf force-pushed the specify_url branch from 1147db5 to d3a1459 Compare March 16, 2017 20:35

tnozicka suggested changes Mar 22, 2017

View reviewed changes

concaf force-pushed the specify_url branch 2 times, most recently from 51639ac to bc90db4 Compare March 23, 2017 09:32

kadel previously approved these changes Mar 27, 2017

View reviewed changes

kadel reviewed Mar 27, 2017

View reviewed changes

concaf force-pushed the specify_url branch from f8a37a6 to bf2a318 Compare March 28, 2017 14:18

kadel reviewed Mar 30, 2017

View reviewed changes

kadel added needs rebase and removed needs-squash labels Mar 30, 2017

concaf force-pushed the specify_url branch 2 times, most recently from b416240 to 56fd227 Compare April 3, 2017 07:03

kadel previously approved these changes Apr 3, 2017

View reviewed changes

concaf force-pushed the specify_url branch from 56fd227 to f4adb19 Compare April 4, 2017 12:47

concaf force-pushed the specify_url branch from f4adb19 to 766cc61 Compare April 4, 2017 12:53

tnozicka reviewed Apr 4, 2017

View reviewed changes

tnozicka approved these changes Apr 4, 2017

View reviewed changes

tnozicka removed the needs rebase label Apr 4, 2017

concaf force-pushed the specify_url branch from 766cc61 to dc64427 Compare April 5, 2017 07:49

kadel approved these changes Apr 5, 2017

View reviewed changes

kadel merged commit d7d01b7 into redhat-developer:master Apr 5, 2017

concaf mentioned this pull request Jul 12, 2017

Take in input from URL kedgeproject/kedge#146

Open

add URL support #64

add URL support #64

Conversation

concaf commented Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf commented Mar 15, 2017

tnozicka commented Mar 15, 2017

concaf commented Mar 15, 2017

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf commented Mar 16, 2017

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf commented Mar 9, 2017 •

edited

Loading

tnozicka Mar 14, 2017 •

edited

Loading

concaf Mar 14, 2017 •

edited

Loading

kadel Mar 27, 2017 •

edited

Loading