Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use volumesForm in definition file #928

Merged
merged 4 commits into from
May 19, 2019
Merged

Use volumesForm in definition file #928

merged 4 commits into from
May 19, 2019

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented May 1, 2019

No description provided.

@antho1404
Copy link
Member

We don't change the attribute in the mesg.yml. Services already depend on this attribute there is no strong reason to change it. This is a breaking change that is not necessary and not the time right now for that.

@antho1404 antho1404 closed this May 1, 2019
@krhubert
Copy link
Contributor Author

krhubert commented May 1, 2019

Everywhere we are using volumesFrom (grpc, json) so having volumesfrom in mesg.yaml is inconsistent so consider again renaming it, as now is the best time because we break a lot in mesg.yaml (like configuration) and we don't have to do breaking change again.

@krhubert krhubert reopened this May 1, 2019
@NicolasMahe
Copy link
Member

Everywhere we are using volumesFrom (grpc, json) so having volumesfrom in mesg.yaml is inconsistent so consider again renaming it, as now is the best time because we break a lot in mesg.yaml (like configuration) and we don't have to do breaking change again.

We didn't break mesg.yml, only the gRPC version and internal version of it. Right?
This is a valid modification that we be merged in another release.

@antho1404
Copy link
Member

Yes, mesg.yml is not broken for now. Also, I think there is a way to parse the yaml with one attribute or the other something like volumefrom|volumeFrom.

The breaking changes of the API are not as critical as the mesg.yml, API is mostly internal (or used with other tools we control such as the js library), We don't have any control on the mesg.yml so this is a critical breaking change and just for cosmetic...

@NicolasMahe
Copy link
Member

Let's include this change in the future break of the task's output

@NicolasMahe
Copy link
Member

NicolasMahe commented May 17, 2019

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

Tested with a service with a dependency that read and write on 2 shared files 👍

@antho1404 antho1404 merged commit 50d901b into dev May 19, 2019
@antho1404 antho1404 deleted the fix/definition-volume branch May 19, 2019 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants