Kedge will read input from stdin #450

surajnarwade · 2017-11-15T07:49:36Z

Resolves #183

For example,

$ cat mariadb.yaml | kedge generate -f -
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  creationTimestamp: null
  labels:
    app: database
  name: database
...
...

cdrage · 2017-11-16T19:59:25Z

Please write integration / unit tests 👍

cdrage · 2017-11-16T20:00:03Z

pkg/cmd/util.go

 			if err != nil {
-				return nil, errors.Wrapf(err, "can't list *.yml files in %s", path)
+				return nil, errors.Wrapf(err, "can't get file info about %s", path)


can't retrieve file information about

github is messed up here, I didn't change anything

I know, but the correct grammar is can't retrieve file information about %s

surajssd

When working with single file it works fine. Like this one works pretty well:

$ cat db.yaml | kedge generate -f - | grep -B 3 "kind"                                  
---
apiVersion: v1
kind: PersistentVolumeClaim
--
status: {}
---
apiVersion: v1
kind: Service
--
  loadBalancer: {}
---
apiVersion: extensions/v1beta1
kind: Deployment

But when I give multiple files, it does not work

$ cat *.yaml | kedge generate -f - | grep -B 3 "kind" 
---
apiVersion: v1
kind: PersistentVolumeClaim
--
status: {}
---
apiVersion: v1
kind: Service
--
  loadBalancer: {}
---
apiVersion: extensions/v1beta1
kind: Deployment

It is still reading only one file.

The right output for $ cat *.yaml | kedge generate -f - | grep -B 3 "kind" should match following.

$ kedge generate -f . | grep -B 3 "kind" 
---
apiVersion: v1
kind: PersistentVolumeClaim
--
status: {}
---
apiVersion: v1
kind: Service
--
  loadBalancer: {}
---
apiVersion: extensions/v1beta1
kind: Deployment
--
status: {}
---
apiVersion: v1
kind: Service
--
  loadBalancer: {}
---
apiVersion: extensions/v1beta1
kind: Deployment

surajnarwade · 2017-11-20T05:08:22Z

@surajssd , if you put --- in every file that you are feeding to kedge, it will give you output as you expected

cdrage · 2017-11-20T17:23:01Z

@surajnarwade Yes, but that's not what's intended nor originally used... You will have to update your function in order to facilitate having multiple files.

kadel · 2017-11-21T10:28:08Z

@surajssd , if you put --- in every file that you are feeding to kedge, it will give you output as you expected

that is not user friendly, we need another solution

surajnarwade · 2017-11-22T05:33:27Z

@kadel how it will distinguish between two files since it's stdin ?

surajnarwade · 2017-11-22T11:29:31Z

@surajssd @kadel @cdrage , I have updated PR as per @surajssd suggestion, can you please have a look

kadel · 2017-11-22T13:31:46Z

pkg/cmd/util.go

+			scanner := bufio.NewScanner(os.Stdin)
+			for scanner.Scan() {
+
+				if strings.Contains(scanner.Text(), "name") && !strings.Contains(scanner.Text(), "-") && !strings.HasPrefix(scanner.Text(), " ") {


hmm, Interesting solution.
Maybe this could be a little bit more elegant if you use regexp instead of 3 tests with string ;-)

We should make sure that there are no unintentional consequences when we are using name for detecting that new file begun. Like for example what happens if name is at the end of the file? That is perfectly valid.

surajssd · 2017-11-23T08:49:59Z

I think we have been over this discussion before of "how to make sure the given kedge format is valid one?".

I think what @surajnarwade is saying is correct there is no way to find out the separation of various kedge files, unless user is writing the yaml files in valid way.

So accoding to yaml specification:

YAML uses three dashes (“---”) to separate directives from document content.

source: http://www.yaml.org/spec/1.2/spec.html#id2760395

So it is user's responsibility to delimit multiple files being streamed in via STDIN

surajnarwade · 2017-11-23T10:15:27Z

@surajssd @kadel , Updated PR, can you please test it again.
@cdrage I didn't find any proper documentation for how to write test for such stdin cases (as in functional/unit/integration test)

cdrage · 2017-11-23T13:28:09Z

@surajnarwade https://stackoverflow.com/questions/37256859/how-can-i-write-a-go-test-that-writes-to-stdin ?

surajnarwade · 2017-11-28T06:57:20Z

@cdrage , I tried that solution but didn't work for me :(
I tried,

    subproc := exec.Command("cat kedge.yml | kedge generate -f -")
    output, _ := subproc.Output()

I am bit confused over it.

cdrage · 2017-11-28T15:23:39Z

@surajnarwade it's not a direct solution, but rather a way to investigate on how to implement it. You would use:

Some pseudocode:

    kedgeFile := `
name: httpd

containers:
- image: centos/httpd

services:
- name: httpd
  type: LoadBalancer
  portMappings: 
    - 8080:80
`
    subproc := exec.Command("cat kedge.yml | ./kedge generate -f -")
    output, _ := subproc.Output()
    if output != kedgeFile {
      return nil
  }

and compare the output(s).

What errors are you getting too?

It'll take some trial-and-error, but I believe it's do-able, let me know if you need any help 👍

cdrage · 2017-12-13T15:05:22Z

New tests LGTM 👍 Code LGTM 👍

cdrage

code lgtm other than small nitpicks

cdrage · 2017-12-13T15:00:31Z

.travis.yml

@@ -25,6 +25,7 @@ install:
  - true

 script:
+  - make bin


why are we adding this? kedge is getting installed anyways within travis 👎

cdrage · 2017-12-13T15:01:48Z

pkg/cmd/util.go

 			if err != nil {
-				return nil, errors.Wrapf(err, "can't list *.yml files in %s", path)
+				return nil, errors.Wrapf(err, "can't get file info about %s", path)


I know, but the correct grammar is can't retrieve file information about %s

surajnarwade · 2017-12-14T08:30:50Z

@cdrage all done

concaf · 2017-12-18T10:44:10Z

pkg/cmd/util.go

-			return nil, errors.Wrapf(err, "cannot determine the absolute file path of %q", file)
+
+		if file == "-" {
+			data, err = ioutil.ReadAll(os.Stdin)


err is not handled here

added error handling :)

surajnarwade · 2017-12-22T06:24:17Z

@containscafeine @cdrage @kadel all green

kadel · 2018-01-04T12:44:13Z

looks good, but needs rebase

surajnarwade · 2018-01-05T08:37:44Z

@kadel @cdrage done with rebase , good to go

kadel

LGTM, but this definitely needs second review

surajnarwade · 2018-01-11T10:36:27Z

@cdrage , review needed :)

cdrage · 2018-01-11T15:43:16Z

@surajnarwade code LGTM. one last rebase and let's merge

Resolves kedgeproject#183 For example, ``` $ cat mariadb.yaml | kedge generate -f - --- apiVersion: v1 kind: PersistentVolumeClaim metadata: creationTimestamp: null labels: app: database name: database ... ... ```

surajnarwade · 2018-01-18T09:40:25Z

@containscafeine final review please and then it's ready to merge

cdrage reviewed Nov 16, 2017

View reviewed changes

surajssd suggested changes Nov 17, 2017

View reviewed changes

surajnarwade force-pushed the stdin branch from 578083a to ab06eb4 Compare November 22, 2017 10:38

kadel reviewed Nov 22, 2017

View reviewed changes

surajnarwade force-pushed the stdin branch from ab06eb4 to 8bf611b Compare November 23, 2017 09:44

surajnarwade force-pushed the stdin branch from 8bf611b to b7a7561 Compare December 11, 2017 13:06

surajnarwade changed the title ~~Kedge will read input from stdin~~ [WIP]Kedge will read input from stdin Dec 11, 2017

surajnarwade force-pushed the stdin branch 7 times, most recently from fd481a9 to f665e79 Compare December 12, 2017 06:56

surajnarwade changed the title ~~[WIP]Kedge will read input from stdin~~ Kedge will read input from stdin Dec 12, 2017

cdrage suggested changes Dec 13, 2017

View reviewed changes

surajnarwade force-pushed the stdin branch 2 times, most recently from 8003138 to 57d4b5f Compare December 14, 2017 08:20

concaf suggested changes Dec 18, 2017

View reviewed changes

surajnarwade force-pushed the stdin branch from 57d4b5f to 990f3b9 Compare December 18, 2017 11:42

kadel added the status/needs-rebase label Jan 4, 2018

surajnarwade force-pushed the stdin branch from 990f3b9 to 78e2b28 Compare January 5, 2018 08:36

surajnarwade removed the status/needs-rebase label Jan 5, 2018

kadel approved these changes Jan 5, 2018

View reviewed changes

cdrage approved these changes Jan 11, 2018

View reviewed changes

Kedge will read input from stdin

36b722d

Resolves kedgeproject#183 For example, ``` $ cat mariadb.yaml | kedge generate -f - --- apiVersion: v1 kind: PersistentVolumeClaim metadata: creationTimestamp: null labels: app: database name: database ... ... ```

surajnarwade force-pushed the stdin branch from 78e2b28 to 36b722d Compare January 16, 2018 08:58

concaf merged commit b86dfbc into kedgeproject:master Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kedge will read input from stdin #450

Kedge will read input from stdin #450

surajnarwade commented Nov 15, 2017

cdrage commented Nov 16, 2017

cdrage Nov 16, 2017

surajnarwade Nov 22, 2017

cdrage Dec 13, 2017

surajssd left a comment

surajnarwade commented Nov 20, 2017

cdrage commented Nov 20, 2017

kadel commented Nov 21, 2017

surajnarwade commented Nov 22, 2017

surajnarwade commented Nov 22, 2017

kadel Nov 22, 2017

surajssd commented Nov 23, 2017

surajnarwade commented Nov 23, 2017

cdrage commented Nov 23, 2017

surajnarwade commented Nov 28, 2017 •

edited

Loading

cdrage commented Nov 28, 2017 •

edited

Loading

cdrage commented Dec 13, 2017

cdrage left a comment

cdrage Dec 13, 2017

cdrage Dec 13, 2017

surajnarwade commented Dec 14, 2017

concaf Dec 18, 2017

surajnarwade Dec 18, 2017

surajnarwade commented Dec 22, 2017

kadel commented Jan 4, 2018

surajnarwade commented Jan 5, 2018

kadel left a comment

surajnarwade commented Jan 11, 2018

cdrage commented Jan 11, 2018

surajnarwade commented Jan 18, 2018

Kedge will read input from stdin #450

Kedge will read input from stdin #450

Conversation

surajnarwade commented Nov 15, 2017

cdrage commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd left a comment

Choose a reason for hiding this comment

surajnarwade commented Nov 20, 2017

cdrage commented Nov 20, 2017

kadel commented Nov 21, 2017

surajnarwade commented Nov 22, 2017

surajnarwade commented Nov 22, 2017

Choose a reason for hiding this comment

surajssd commented Nov 23, 2017

surajnarwade commented Nov 23, 2017

cdrage commented Nov 23, 2017

surajnarwade commented Nov 28, 2017 • edited Loading

cdrage commented Nov 28, 2017 • edited Loading

cdrage commented Dec 13, 2017

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Dec 22, 2017

kadel commented Jan 4, 2018

surajnarwade commented Jan 5, 2018

kadel left a comment

Choose a reason for hiding this comment

surajnarwade commented Jan 11, 2018

cdrage commented Jan 11, 2018

surajnarwade commented Jan 18, 2018

surajnarwade commented Nov 28, 2017 •

edited

Loading

cdrage commented Nov 28, 2017 •

edited

Loading