feat(spec): add support for extraResources #173

surajssd · 2017-07-24T07:30:53Z

With this root level field called extraResources, which is a list
of file names of Kubernetes artifacts that can be fed to Kubernetes
directly without kedge having to do any processing.

Anything that is not supported in kedge can be provided using this
field. Following is the example snippet of the app file that is
using extraResources field to deploy a secret.yaml and a
cron-job.yaml with the rest of kedge file. Here secret.yaml &
cron-jobs.yaml reside in the same directory.

e.g.

...
extraResources:
- secret.yaml
- cron-jobs.yaml

Fixes #157

kedge-bot · 2017-07-24T07:30:55Z

@surajssd, thank you for the pull request! We'll ping some people to review your PR. @containscafeine, please review this.

surajssd · 2017-07-24T07:32:44Z

TODO:

Add README to the example directory
Add to the file refernce doc about this

surajssd · 2017-07-24T07:39:23Z

pkg/transform/kubernetes/kubernetes.go

 	api_v1 "k8s.io/client-go/pkg/api/v1"
 	ext_v1beta1 "k8s.io/client-go/pkg/apis/extensions/v1beta1"
+	"k8s.io/client-go/pkg/runtime"
+	"k8s.io/client-go/pkg/util/intstr"


this happened according to this #149 (comment)

surajssd · 2017-07-24T09:57:53Z

blocked on #172

surajssd · 2017-07-24T11:56:50Z

since #172 is merged this is not blocked anymore and open to review

surajssd · 2017-07-26T05:43:36Z

ping: @containscafeine @kadel

surajssd · 2017-07-27T14:02:07Z

@containscafeine rebased!

kadel · 2017-07-27T14:36:46Z

examples/extraResources/README.md

+Kedge might not support all the things that Kubernetes has, but kedge would not
+come in your way to define anything that Kubernetes understands.
+
+For e.g. right now there is no way to define secrets in kedge, but you can still


I think we can't remove those lines, as there is already is PR for secrets.

Some alpha features like CronJob might be better example

changed the example.

kadel · 2017-07-28T11:37:53Z

pkg/transform/kubernetes/kubernetes.go

@@ -334,7 +334,7 @@ func populateEnvFrom(app *spec.App) error {
 	return nil
 }

-func CreateK8sObjects(app *spec.App) ([]runtime.Object, error) {
+func CreateK8sObjects(app *spec.App) ([]runtime.Object, []string, error) {


I think that it would be good to add comment explaining what this does and what each return value is ([]runtime.Object, []string)

kadel · 2017-07-28T11:39:37Z

pkg/transform/kubernetes/kubernetes.go

 }

-func Transform(app *spec.App) ([]runtime.Object, error) {
+func Transform(app *spec.App) ([]runtime.Object, []string, error) {


Same here as for CreateK8sObjects return values of those function are getting more complicated and it would be good to have a comment explaining it.

kadel · 2017-07-28T11:40:57Z

Just a few missing comments and doc fix, otherwise it looks good to me 👍

surajssd · 2017-07-31T08:58:08Z

ping @containscafeine

concaf

will review code in a bit

concaf · 2017-07-31T09:45:25Z

docs/file-reference.md

+
+The file path are relative to the kedge application file.
+
+This is one of the mechanism to extend kedge beyond it's capabilites to support


one of the mechanisms

concaf · 2017-07-31T09:46:16Z

examples/extraResources/README.md

+Kedge might not support all the things that Kubernetes has, but kedge would not
+come in your way to define anything that Kubernetes understands.
+
+For e.g. right now there is no way to define cron-jobs in kedge, but you can still


s/cron-jobs/Kubernetes cron jobs

concaf · 2017-07-31T09:46:58Z

examples/extraResources/README.md

+come in your way to define anything that Kubernetes understands.
+
+For e.g. right now there is no way to define cron-jobs in kedge, but you can still
+specify the Kubernetes cron-job file. In this field called `extraResources`.


s/cron-job/cron job

concaf · 2017-07-31T09:47:35Z

examples/extraResources/README.md

+```
+
+So in this way you can specify list of files under root level field called
+`extraResources`. These files has configuration that Kubernetes understands,


concaf · 2017-07-31T09:48:33Z

examples/extraResources/README.md

+Also the file paths under `extraResources` should be relative to the kedge file
+in which this config is specified.
+
+This does not change anything with respect to deploying application.


s/application/applications

surajssd · 2017-07-31T11:51:12Z

@containscafeine changed all of it!

kadel

This looks good to me.
But let's wait for @containscafeine approval before merge

concaf · 2017-07-31T14:22:53Z

pkg/transform/kubernetes/kubernetes.go

 	if err != nil {
-		return nil, errors.Wrap(err, "failed to create Kubernetes objects")
+		return nil, extraResources, errors.Wrap(err, "failed to create Kubernetes objects")


umm, any reason we are returning return nil, extraResources, "error" and not return nil, nil, "error") here and everywhere else?

concaf

Also, this fails in case of passing directory to -f-.

Running kedge generate -f examples/extraResources/ it returns unable to transform data: No runtime objects created, possibly because not enough input data was passed with a non zero exit code.

If a file has been mentioned in the extraResources field, we need to not pass it further in the code.

Passing absolute paths fail, which should not be the case.

Putting this in the file -

extraResources:
- /tmp/cronjob.yaml

and running $ kedge generate -f examples/extraResources/app.yaml, I get file reading failed: open /home/concaf/kedge/src/github.com/kedgeproject/kedge/examples/extraResources/tmp/cronjob.yaml: no such file or directory
filepath.IsAbs should be able to help us with that - https://golang.org/pkg/path/filepath/#IsAbs

concaf · 2017-07-31T15:15:21Z

fwiw, I did similar stuff in OpenCompose in this part - https://github.com/containscafeine/opencompose/blob/95d568b90965ae952d08593ea16702477d914dc5/pkg/encoding/v1/encoding.go#L604-L627

kadel · 2017-07-31T15:48:11Z

Running kedge generate -f examples/extraResources/ it returns unable to transform data: No runtime objects created, possibly because not enough input data was passed with a non zero exit code.

This fails if kedge files are in the same directory with Kubernetes resources. They both are YAML files. -f reads all *.yml and *.yaml files from the directory. It doesn't know what is Kubernetes file and what is Kedge files, so all the files are treated as Kedge files.

Kubernetes files should be placed in a different directory than Kedge files.
This is something that we already discussed somewhere.

examples/extraResources
├── app.yaml
├── kubernetes
│   └── cronjob.yaml
└── README.md

extraResources:
- kubernetes/cronjob.yaml

now kedge apply -f examples/extraResources is going to work.

I'm not sure if ignoring files form extraResources is going to be possible, as you have to parse the file first to get extraResources array. And to parse it you have to know if its Kedge file or not ;-)

concaf · 2017-07-31T15:51:28Z

@kadel agreed, but this is a corner case, no? How about we add a check which makes sure that if the file passed in extraResources is placed in the same directory, then we don't parse it?
We won't check if the file is valid Kubernetes or not, just that if it's present in the current directory, it's ignored.

kadel · 2017-07-31T15:52:11Z

I'm not sure if ignoring files form extraResources is going to be possible, as you have to parse the file first to get extraResources array. And to parse it you have to know if its Kedge file or not ;-)

kadel · 2017-07-31T15:53:23Z

Problem is that also what error you throw. As its not possible to know if meant to be Kubernetes file or invalid Kedge file

surajssd · 2017-07-31T16:12:05Z

Passing absolute paths fail, which should not be the case.

~~@containscafeine we will support only absolute paths~~

Ahh my bad, yeah I meant that we will support only relative paths, because absolute paths make the config tied to a machine, which is not the case, so not allowing also makes sense.

concaf · 2017-07-31T16:21:53Z

@surajssd disagree, it has to be left to the user. It's the convention used everywhere. e.g. any linux command, Dockerfile, Docker Compose files, Kubernetes volume mount paths, etc, etc.
Also, it's very confusing since the user tends to pass both relative and absolute paths in any path field.

surajssd · 2017-07-31T16:33:21Z

@containscafeine updated the comment and code!

concaf · 2017-07-31T16:34:55Z

@kadel maybe for this PR we can ignore the issue with Kubernetes files in the same directory, but I think it's worth adding an edge case check here some time later. We can track this as a bug in a separate issue, wdyt?

surajssd · 2017-08-01T11:45:39Z

Passing absolute paths fail, which should not be the case.

@containscafeine added can you check if it works for you

With this root level field called `extraResources`, which is a list of file names of Kubernetes artifacts that can be fed to Kubernetes directly without kedge having to do any processing. Anything that is not supported in kedge can be provided using this field. Following is the example snippet of the app file that is using `extraResources` field to deploy a `secret.yaml` and a `cron-job.yaml` with the rest of kedge file. Here `secret.yaml` & `cron-jobs.yaml` reside in the same directory. e.g. ``` ... extraResources: - secret.yaml - cron-jobs.yaml ```

concaf

great work, @surajssd 🎉

kedge-bot requested a review from concaf July 24, 2017 07:30

surajssd force-pushed the k8s-artifacts-passing branch from ebd5eaa to 05b3bd3 Compare July 24, 2017 07:38

surajssd commented Jul 24, 2017

View reviewed changes

surajssd force-pushed the k8s-artifacts-passing branch from 05b3bd3 to d9ecf79 Compare July 24, 2017 11:32

surajssd added the status/needs-review label Jul 24, 2017

surajssd force-pushed the k8s-artifacts-passing branch 2 times, most recently from e1e9dc0 to b0006fe Compare July 24, 2017 11:55

surajssd requested a review from kadel July 24, 2017 11:56

surajssd force-pushed the k8s-artifacts-passing branch from b0006fe to e198665 Compare July 27, 2017 04:13

concaf added the status/needs-rebase label Jul 27, 2017

surajssd force-pushed the k8s-artifacts-passing branch from e198665 to 387e0e8 Compare July 27, 2017 13:57

surajssd removed the status/needs-rebase label Jul 27, 2017

kadel reviewed Jul 27, 2017

View reviewed changes

kadel reviewed Jul 28, 2017

View reviewed changes

surajssd force-pushed the k8s-artifacts-passing branch 2 times, most recently from 3f76f88 to 3c8b711 Compare July 31, 2017 04:16

concaf suggested changes Jul 31, 2017

View reviewed changes

surajssd force-pushed the k8s-artifacts-passing branch 2 times, most recently from badd405 to 0b97b3f Compare July 31, 2017 11:50

kadel approved these changes Jul 31, 2017

View reviewed changes

concaf reviewed Jul 31, 2017

View reviewed changes

concaf suggested changes Jul 31, 2017

View reviewed changes

concaf removed the status/needs-review label Jul 31, 2017

surajssd added the status/needs-rebase label Jul 31, 2017

surajssd force-pushed the k8s-artifacts-passing branch from 0b97b3f to 64a5fae Compare July 31, 2017 16:25

surajssd removed the status/needs-rebase label Jul 31, 2017

surajssd force-pushed the k8s-artifacts-passing branch from 64a5fae to 6a9cd13 Compare August 1, 2017 11:45

surajssd added the status/needs-review label Aug 1, 2017

surajssd force-pushed the k8s-artifacts-passing branch from 6a9cd13 to f4053c4 Compare August 1, 2017 12:33

concaf approved these changes Aug 1, 2017

View reviewed changes

concaf merged commit 6cb69fe into kedgeproject:master Aug 1, 2017

surajssd mentioned this pull request Sep 13, 2017

docs: define go import convention #258

Merged


		The file path are relative to the kedge application file.

		This is one of the mechanism to extend kedge beyond it's capabilites to support

feat(spec): add support for extraResources #173

feat(spec): add support for extraResources #173

Conversation

surajssd commented Jul 24, 2017

kedge-bot commented Jul 24, 2017

surajssd commented Jul 24, 2017 • edited Loading

surajssd Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

surajssd commented Jul 24, 2017

surajssd commented Jul 24, 2017

surajssd commented Jul 26, 2017

surajssd commented Jul 27, 2017

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

kadel commented Jul 28, 2017

surajssd commented Jul 31, 2017

concaf 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

surajssd commented Jul 31, 2017

kadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf left a comment • edited Loading

Choose a reason for hiding this comment

concaf commented Jul 31, 2017

kadel commented Jul 31, 2017 • edited Loading

concaf commented Jul 31, 2017

kadel commented Jul 31, 2017

kadel commented Jul 31, 2017 • edited Loading

surajssd commented Jul 31, 2017 • edited Loading

concaf commented Jul 31, 2017 • edited Loading

surajssd commented Jul 31, 2017

concaf commented Jul 31, 2017

surajssd commented Aug 1, 2017

concaf left a comment

Choose a reason for hiding this comment

surajssd commented Jul 24, 2017 •

edited

Loading

surajssd Jul 24, 2017 •

edited

Loading

concaf left a comment •

edited

Loading

kadel commented Jul 31, 2017 •

edited

Loading

kadel commented Jul 31, 2017 •

edited

Loading

surajssd commented Jul 31, 2017 •

edited

Loading

concaf commented Jul 31, 2017 •

edited

Loading