-
Notifications
You must be signed in to change notification settings - Fork 68
proto: improve comments #67
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
proto/manifest.proto
Outdated
| // Path specifies the path from the bundle root. If more than one | ||
| // path is present, the entry may represent a hardlink, rather than using | ||
| // a link target. The path format is operating system specific. | ||
| // Path MAY be an absolute path, but it MUST be interpreted with the bundle root. |
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.
current manifest_test.go uses relative path but continuity build uses absolute path.
Suggestion: can we prohibit absolute path for ease of robust implementation?
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.
@stevvooe WDYT?
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.
Do we need to have a change to make this work, as well?
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.
We should always be using relative paths for continuity.
proto/manifest.proto
Outdated
| // On Unix, gid MUST be a positive decimal integer or an empty string. | ||
| string gid = 3; | ||
|
|
||
| // User and group specify the user name and the group name for the resource. |
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.
These fields aren't used, so this definition is meaningless.
proto/manifest.proto
Outdated
| // Uid specifies the user id for the resource. A string type is used for | ||
| // compatibility across different OS. | ||
| // On Unix, uid MUST be a positive decimal integer or an empty string. | ||
| string uid = 2; |
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.
Let's just change these to integers. Windows aren't gonna use these, so they'll always be integers. Use int64.
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.
- not uint64?
- please elaborate your plan for windows?
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 elaborate your plan for windows?
You'll have to ask the microsoft folks, but they made the indication that we are going to use separate fields for that purpose. The only reason these are strings is due to the fact Windows has string user ids.
not uint64?
Don't use integer types to enforce signedness.
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.
cc @jhowardmsft
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.
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.
thank you, will update soon
|
updated PR and opened #77 for unstringifying uid&gid |
|
@AkihiroSuda Paths should be relative. |
|
ok, I will open a PR in a few days |
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
|
updated PR, and opened #82 |
|
LGTM |
Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp