-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-4951: conversion of service files #174
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
Conversation
|
Requested publication of version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the "coverage" issues
|
Wait ENG-4950 |
|
Is the svc files supposed to be in the bundle? Because there only present in the src project that not always contains the bundle descriptors. So, I was thinking we focus on the delivered v1 bundle that doesn't contain the svc, for sure |
In Bundle URI, you probably won't find them, but that's an optional step where the svc path folder is a user input and the user can specify the path if they are used locally wherever they are. CC: @firegloves |
|
Requested publication of version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the "coverage" issues
|
@antromeo thanks for the further information. It makes sense now, I would suggest making the prompt more obvious (maybe give more details in the prompt sentence). As you said, if we also support the sources Bundle, it will be more useful because we are almost sure to have them. Another solution would be to ask the user if he/she wants to add the svc and then ask for the path. |
|
@avdev4j in my opinion two questions are maybe too verbose. |
src/commands/convert.ts
Outdated
|
|
||
| // adds services | ||
| servicePath = servicePath ?? await CliUx.ux.prompt( | ||
| 'Services path (optional)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'Services path (optional)', | |
| 'Please provide the path to the service files source code (optional)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion from @jyunmitch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avdev4j, @jyunmitch
just a note: what do you think if we took away the words "source code"? because docker compose files are actually yaml files (no source)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work? "Please provide the path to the YAML service files (optional)"
or "Please provide the path to the Docker Compose service files (optional)"
|
Requested publication of version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the "coverage" issues
|
Requested publication of version |
No description provided.