This will add a new key appversion #454

piyush-garg · 2017-11-15T13:32:15Z

This will add a new key appversion which will get reflected in the
metadata(as key-pair in annotation) of deployment, job and
deploymentConfig. It is optional and also empty value of appversion
will not be added to metadata #407

kadel · 2017-11-15T15:09:31Z

Can you please add docs, and tests?

kadel · 2017-11-16T12:09:00Z

pkg/spec/types.go

@@ -144,6 +145,10 @@ type SecretMod struct {

 // ControllerFields are the common fields in every controller Kedge supports
 type ControllerFields struct {
+	// Field to specify the version of application
+	// +optional
+	Appversion json.Number `json:"appversion,omitempty"`


It would be better to have Appversion as a string. We should allow appversions like 1.0.4-beta1

kadel · 2017-11-16T12:14:24Z

pkg/spec/deployment.go

@@ -56,6 +56,9 @@ func (deployment *DeploymentSpecMod) Fix() error {

 	deployment.ControllerFields.ObjectMeta.Labels = addKeyValueToMap(appLabelKey, deployment.ControllerFields.Name, deployment.ControllerFields.ObjectMeta.Labels)

+	if deployment.ControllerFields.Appversion != "" {
+		deployment.ControllerFields.ObjectMeta.Annotations = addKeyValueToMap("appversion", string(deployment.ControllerFields.Appversion), deployment.ControllerFields.ObjectMeta.Annotations)


appversion should be a constant. Similarly to appLabelKey

If you redefine Appversions as a string you won't have to do type conversion

We should add this label to every object created from the file.
It might be better to do this together for all objects. This could be done at the end of the CreateK8sObjects function in the resourece.go , just before all objects are returned. You might have to do some type conversion magic to get ObjectMeta from runtime.Object ;-)

specifically this could be:

appVersion

With V being capital

just before all objects are returned. You might have to do some type conversion magic to get ObjectMeta from runtime.Object ;-)

@kadel can't we just do this label addition in each create* function? Like before we add ObjectMeta in places [1], [2], [3].

Can we have some way where we can enforce the call of function which does addition of this label?

surajssd · 2017-11-20T04:05:45Z

pkg/spec/resources.go

+				objects[i] = t
+			case *api_v1.ConfigMap:
+				t.ObjectMeta.Annotations = addKeyValueToMap(appVersion, app.Appversion, t.ObjectMeta.Annotations)
+				objects[i] = t


@kadel the problem with this approach is that there is no guarantee we might not miss out on new object!

If tomorrow kedge starts supporting a new object and if we miss out on adding that here, we will have same problem as to calling addKeyValueToMap in some fix* method or in some create* method.

I think there is a solution to this, if we have a default case where anything that we don't handle will come and we show it with big warning saying that this particular kind was not handled in this type switch!

Having default is good practice and should be there! We should look at the solution where we don't even need type switch. Some simple unified way that will add labels to any runtime object regarding the type.

surajssd · 2017-11-20T04:07:14Z

pkg/spec/resources.go

+	if app.Appversion != "" {
+		for i, object := range objects {
+			switch t := object.(type) {
+			case *ext_v1beta1.Ingress:


@piyush1594 all these cases can be clubbed together in one case

case *ext_v1beta1.Ingress, *os_route_v1.Route, *api_v1.Service, ..., *api_v1.ConfigMap:

And since we are doing same thing in all cases, so we can add them together!

surajssd · 2017-11-20T07:16:50Z

I think the right place to call this function where you are adding all these annotations/labels is in CoreOperations before return, this is the last place where we are doing anything on the data we have generated.

But another question that arises is that should we also be playing with includeResources also?

kadel · 2017-11-20T08:42:34Z

But another question that arises is that should we also be playing with includeResources also?

Hmm, good question. Ideally yes. But it will require changing how we handle includeResources.

kadel · 2017-11-20T10:42:08Z

I think the right place to call this function where you are adding all these annotations/labels is in CoreOperations before return, this is the last place where we are doing anything on the data we have generated.

Yes, that might be even better place.

cdrage · 2017-11-21T15:15:05Z

Commits need to be merged / be one as well 👍

kadel · 2017-11-22T07:36:41Z

pkg/spec/types.go

@@ -162,6 +162,10 @@ type BuildConfigSpecMod struct {

 // ControllerFields are the common fields in every controller Kedge supports
 type ControllerFields struct {
+	// Field to specify the version of application
+	// +optional
+	Appversion string `json:"appversion,omitempty"`


Hi @piyush1594. You can use intstr.IntOrString from k8s.io/apimachinery/pkg/util/intstr, check out #455 for inspiration ;-)

Hey @kadel

I tried that function. It is working fine if input is string but if input is int, output is in quotes. And if nothing is in input, output is giving "0".

And it gives error in case of float like appversion : 5.6

@piyush1594 I think it is throwing "0" because it is converting the zero value of an int (which is 0) to a string.
One solution would be to make the type *intstr.IntOrString (because zero value of a pointer is going to be nil) and then check the value of AppVersion is nil or not. If it is nil, this means that the user has not set the field.
However, I think that the internal implementation of instr.IntOrString is a pointer anyway, so you can directly check if it is nil or not, not sure about this though.

@kadel for float values like 5.6, I think we should have something like FloatOrString, WDYT?

@containscafeine Using pointer it resolved zero value problem. Thanks

@kadel for float values like 5.6, I think we should have something like FloatOrString, WDYT?

Hmm, good point. using IntOrString is not going to solve the problem when someone puts something like 5.6 (without quotes) :-(

lets check apimachinery what other types are there besides IntOrString , I bet that we will find something usefull

Hmm, more and more I think about this. I would like to stop with this and just require appversion to be put in quotes as a regular string. 😬

We could have the same discussion for every field. This just doesn't make sense. Let's use string.
If the field is required to be a string then it has to be a string and for numbers, quotes are required.

If someone sets label the value to float 5.5 or 5 without quotes, Kubernetes will also fail (so does Kedge) and that is correct.

surajssd · 2017-11-22T12:27:27Z

pkg/spec/resources.go

+				return nil, nil, errors.Wrap(err, "cannot set annotations")
+			}
+		}
+	}


this needs to move to CoreOperations right ?

Also you can put it in a function so, there won't be a need to have this code in between!

@surajssd I tried it there but I was not able to get the value of appversion there, can you please suggest me some method to do that.

One I thought was that we can return the value of appversion along with all runtimeObjects

surajssd · 2017-11-22T12:28:16Z

pkg/spec/deployment.go

@@ -56,6 +56,9 @@ func (deployment *DeploymentSpecMod) Fix() error {

 	deployment.ControllerFields.ObjectMeta.Labels = addKeyValueToMap(appLabelKey, deployment.ControllerFields.Name, deployment.ControllerFields.ObjectMeta.Labels)

+	if deployment.ControllerFields.Appversion != "" {
+		deployment.ControllerFields.ObjectMeta.Annotations = addKeyValueToMap(appVersion, deployment.ControllerFields.Appversion, deployment.ControllerFields.ObjectMeta.Annotations)
+	}


If you do #454 (comment) this is not needed!

surajssd · 2017-11-22T12:28:38Z

pkg/spec/deployment.go

@@ -134,6 +137,8 @@ func (deployment *DeploymentSpecMod) createKubernetesController() (*ext_v1beta1.
 	// TODO: merge with already existing labels and avoid duplication
 	deploymentSpec.Template.ObjectMeta.Labels = deployment.Labels

+	deploymentSpec.Template.ObjectMeta.Annotations = deployment.Annotations


If you do #454 (comment) this is not needed!

surajssd · 2017-11-22T12:28:48Z

pkg/spec/deploymentconfig.go

+		deploymentConfig.ControllerFields.ObjectMeta.Annotations = addKeyValueToMap(appVersion,
+			deploymentConfig.ControllerFields.Appversion,
+			deploymentConfig.ControllerFields.ObjectMeta.Annotations)
+	}


If you do #454 (comment) this is not needed!

surajssd · 2017-11-22T12:28:57Z

pkg/spec/job.go

+
+	if job.ControllerFields.Appversion != "" {
+		job.ControllerFields.ObjectMeta.Annotations = addKeyValueToMap(appVersion, job.ControllerFields.Appversion, job.ControllerFields.ObjectMeta.Annotations)
+	}


If you do #454 (comment) this is not needed!

THis can't be moved as that comment suggested, there is information about Kedge file structures in context of that function

kadel · 2017-11-22T13:47:22Z

We have to figure out this in general. This is a second global label/annotation (we already have app label), having same lines in multiple places (as it is done with app label) is not maintainable and CoreOperation is not a good place for it (and it is not even possible currently)

This might require a bit of refactoring and handling artifacts generations in a better way

surajssd · 2017-11-23T08:39:11Z

@kadel yeah even I realize that, not sure right now what is the best way to do it!

If you want we can extract the appVersion from the file, like we do for the controller, we do it for the appVersion. Unmarshal into a throw away struct only to extract the field value of appVersion, not sure if we wanna do that! This way the fucntion call can still remain in the CoreOperations.

kadel · 2017-11-23T08:43:13Z

I would keep this as it is for now. At least all non-controller objects are done together.
We can refactor this later (and we will have to)

surajssd

LGTM, with caution of doing refactor soonish!

kadel · 2017-11-23T10:04:42Z

@surajssd what do you think about #454 (comment) do you agree?

surajssd · 2017-11-24T06:51:28Z

@kadel string is what I would like to see, removing all the complications which come with overloading of types.

piyush-garg · 2017-11-24T07:08:02Z

@surajssd if we make it string then user have to give input in quotes like "5.6" This is what @cdrage doesn't want as all other fields are without quotes link.
So, I implemented this. This will also solve problem of doing at one place in CoreOperations

kadel · 2017-11-24T10:26:08Z

pkg/spec/types.go

@@ -163,6 +163,10 @@ type BuildConfigSpecMod struct {

 // ControllerFields are the common fields in every controller Kedge supports
 type ControllerFields struct {
+	// Field to specify the version of application
+	// +optional
+	Appversion *AppVersionType `json:"appversion,omitempty"`


why not pointer to string directly? *string?

kadel · 2017-11-24T10:27:31Z

pkg/spec/util.go

+//this will just convert whatever input given as appversion to string
+//and set the value of appversion so it will not give error at the time
+//of unmarshalling
+func (fs *AppVersionType) UnmarshalJSON(value []byte) error {


we don't need to do this, it will be defined as nil even without this

kadel · 2017-11-24T10:32:46Z

pkg/spec/controller.go

@@ -96,5 +99,44 @@ func CoreOperations(data []byte) ([]runtime.Object, []string, error) {
 		return nil, nil, errors.Wrap(err, "unable to transform data")
 	}

+	//Regex to find the location of appversion


If I understand it correctly this is basically manually parsing appversion from YAML. This is not nice.
Please leave it as it was before. It is better to set labels on multiple places than parsing it here like this.

@kadel I did it because after that user can give input without quotes and also trailing zeroes and all the problem get solved.
I tried IntOrFloarOrString there I was facing trailing zeroes in float problem, otherwise we can use string and user have to give input in quotes

kadel · 2017-11-24T12:13:48Z

@surajssd if we make it string then user have to give input in quotes like "5.6" This is what @cdrage doesn't want as all other fields are without quotes link.

Lets just do it as a regular string as we do it in other fields. If we are going to do some hack around that for appversion than we should do the same for all string fields in Kedge. And that we can't do because Kubernets structures are not done in that way.

This is different case that portMappings just leave this as string without any hack around it.

We will fix the problem with setting labels in multiple places later without requiring hacks

metadata(as key-pair in annotation) of deployment, job and deploymentConfig. It is optional and also empty value of `appversion` will not be added to metadata kedgeproject#407 Formatted code according to gofmt Added annotations to all the objects in resources.go, changed the type of appversion to string, declared a constant for appversion Formatted according to gofmt Added generalized method for all objects and removed type switch Formatted according to gofmt This will accpet input of appversion of any kind like integer or float or string without quotes Here using a special type to allow marshalling and git commit -am Fallback to previous implementation, using Type as string and adding annotations in different places and user have to give input in quotes in case of int or float

kadel

LGTM

kadel · 2017-11-27T11:16:28Z

Merging, as this is the version of this PR that was already approved by @surajssd

kadel suggested changes Nov 16, 2017

View reviewed changes

piyush-garg force-pushed the add_field_appversion branch from 5563446 to c3ebba4 Compare November 17, 2017 16:16

piyush-garg changed the title ~~This will add a new key appversion~~ [WIP] This will add a new key appversion Nov 17, 2017

piyush-garg changed the title ~~[WIP] This will add a new key appversion~~ "[WIP] This will add a new key appversion" Nov 17, 2017

piyush-garg changed the title ~~"[WIP] This will add a new key appversion"~~ [WIP] This will add a new key appversion Nov 17, 2017

surajssd reviewed Nov 20, 2017

View reviewed changes

kadel mentioned this pull request Nov 21, 2017

Allow the usage of a string or int as a value in portMappings #455

Merged

piyush-garg force-pushed the add_field_appversion branch from bbcfc9c to 3115f46 Compare November 21, 2017 12:29

kadel reviewed Nov 22, 2017

View reviewed changes

surajssd suggested changes Nov 22, 2017

View reviewed changes

kadel approved these changes Nov 23, 2017

View reviewed changes

kadel mentioned this pull request Nov 23, 2017

This will add description about the new field appversion #461

Merged

surajssd approved these changes Nov 23, 2017

View reviewed changes

kadel mentioned this pull request Nov 23, 2017

Refactor how we handle global labels and annotations #485

Open

piyush-garg force-pushed the add_field_appversion branch from 3115f46 to c916a18 Compare November 23, 2017 19:58

kadel suggested changes Nov 24, 2017

View reviewed changes

piyush-garg force-pushed the add_field_appversion branch from c916a18 to 86b4f7b Compare November 24, 2017 13:00

piyush-garg force-pushed the add_field_appversion branch from 86b4f7b to b8a4279 Compare November 24, 2017 13:08

piyush-garg changed the title ~~[WIP] This will add a new key appversion~~ This will add a new key appversion Nov 24, 2017

kadel approved these changes Nov 27, 2017

View reviewed changes

kadel merged commit 272f1a6 into kedgeproject:master Nov 27, 2017

This will add a new key appversion #454

This will add a new key appversion #454

Conversation

piyush-garg commented Nov 15, 2017

kadel commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd Nov 17, 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

surajssd commented Nov 20, 2017

kadel commented Nov 20, 2017

kadel commented Nov 20, 2017

cdrage commented Nov 21, 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 Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

kadel Nov 22, 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

kadel commented Nov 22, 2017

surajssd commented Nov 23, 2017

kadel commented Nov 23, 2017 • edited Loading

surajssd left a comment

Choose a reason for hiding this comment

kadel commented Nov 23, 2017

surajssd commented Nov 24, 2017

piyush-garg commented Nov 24, 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

kadel commented Nov 24, 2017

kadel left a comment

Choose a reason for hiding this comment

kadel commented Nov 27, 2017

surajssd Nov 17, 2017 •

edited

Loading

kadel Nov 22, 2017 •

edited

Loading

kadel Nov 22, 2017 •

edited

Loading

kadel commented Nov 23, 2017 •

edited

Loading