-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix the incorrect config path in the documentation #416
Conversation
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.
Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @steed924)
proto/com/github/trace_machina/native_link/remote_execution/worker_api.proto
line 88 at r1 (raw file):
/// /// The details on how to use this property can be found here: /// https://github.com/tracemachina/native-link/blob/master/native-link-config/src/cas_server.rs
nit: We need to pin the hash or else the code can change and make this comment link not work.
proto/genproto/com.github.trace_machina.native_link.remote_execution.pb.rs
line 45 at r1 (raw file):
/// / /// / The details on how to use this property can be found here: /// / <https://github.com/tracemachina/native-link/blob/master/native-link-config/src/cas_server.rs>
ditto.
2c1a098
to
3147265
Compare
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.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @allada)
proto/com/github/trace_machina/native_link/remote_execution/worker_api.proto
line 88 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: We need to pin the hash or else the code can change and make this comment link not work.
done
proto/genproto/com.github.trace_machina.native_link.remote_execution.pb.rs
line 45 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
ditto.
done
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada)
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.
Thanks for catching these!
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @allada)
661431c
to
4198ebc
Compare
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.
LGTM. I wonder if we should one day maintain a jsonc
file that is a copy of all the json
files. That way, we dan have one representation that is comment compatible so that our users do not need to edit the JSON but can still understand the reference jsonc
's.
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @steed924)
Description
After the rewrite of the build infrastructure, all path problems are resolved successfully in code files.
But we missed some of them in the documentation.
Updating documentation needs to be done since it needs to be read by users.
Those documentation issues are fixed.
Fixes #415
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...
passes locallygit amend
see some docsThis change is