Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

This will add description about the new field appversion #461

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

piyush-garg
Copy link
Collaborator

This will add description about the new field appversion
in file-reference.md
#407

@piyush-garg piyush-garg changed the title This will add description about the new field appversion [WIP} This will add description about the new field appversion Nov 17, 2017
@piyush-garg piyush-garg changed the title [WIP} This will add description about the new field appversion [WIP] This will add description about the new field appversion Nov 17, 2017
Copy link
Collaborator

@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.

Please add "appversion" to the serction of the file reference that says: "All defineable Kedge keys"

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don't change this line of code.


| Type | Required | Description |
|----------|--------------|--------------|
| string | yes | The version of the app or micro-service this particular file defines. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

no, it's not required.

## appversion

```yaml
appversion: "5.0.0-alpha"
Copy link
Collaborator

Choose a reason for hiding this comment

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

honestly, i'd put something like 0.0.1 instead of putting it in quotes. looks better / much easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cdrage I put it in quotes because, if someone is giving version as 5 or 5.1 then the user needs to give in quotes, it will work without quotes only in case of a string.
So I don't know what I should finally take as an example.

Copy link
Member

@kadel kadel Nov 20, 2017

Choose a reason for hiding this comment

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

Let's use quotes for now (to be save that this is really a string), and than we can change is based on how #407 will be implemented

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ugly to have quotes... The power of YAML is that it can be either one when unmarshaling.

Based on when #455 is merged, we can make it whatever we want it to be (int or string) if someone were to put a float or a version value.

Copy link
Member

Choose a reason for hiding this comment

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

We can't merge this before #407 anyway, so let's wait for how it will be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

@piyush-garg piyush-garg force-pushed the doc_add_field_appversion branch 2 times, most recently from 86bc1e7 to 4bfc292 Compare November 24, 2017 13:12
@piyush-garg piyush-garg changed the title [WIP] This will add description about the new field appversion This will add description about the new field appversion Nov 24, 2017
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@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.

one small change, otherwise LGTM.

@@ -136,6 +137,7 @@ Each "app" (Kedge file) is a Kubernetes <a target="_blank" href="https://v1-6.do
| Field | Type | Required | Description |
|----------|----------|--------------|--------------|
| name | string | yes | The name of the app or micro-service this particular file defines. |
| appversion | string | no | The version of the app or micro-service this particular file defines. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

microservice is one word

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hi @cdrage,

If you check name and controller field in the same table, it is written like this, if you still want the change then I will do.

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, both need to be changed 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

pushed a PR here: #493

@piyush-garg piyush-garg force-pushed the doc_add_field_appversion branch from 4bfc292 to 31cb13f Compare November 27, 2017 17:58
in file-reference.md

This will add new key appversion in keys section
and fix all the typos

Fixed Typo
@piyush-garg piyush-garg force-pushed the doc_add_field_appversion branch from 31cb13f to c1d83eb Compare November 28, 2017 05:09
@cdrage
Copy link
Collaborator

cdrage commented Nov 28, 2017

LGTM

@cdrage cdrage merged commit 0e1f38d into kedgeproject:master Nov 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants