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

Document contributing to the native-cli #1625

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

aaronmondal
Copy link
Member

@aaronmondal aaronmondal commented Mar 14, 2025

This change is Reviewable

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

+@MarcusSorealheis +@kubevalet human-readable at https://nativelink-y3s9gakb9b1k.deno.dev/docs/contribute/native-cli

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 3 files reviewed, and pending CI: Bazel Dev / macos-15, Cargo Dev / macos-15, Installation / macos-14, Installation / macos-15, Local / lre-rs / macos-15, NativeLink.com Cloud / Remote Cache / macos-15, Web Platform Deployment / macos-15, macos-15 (waiting on @kubevalet and @MarcusSorealheis)

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

My concern is less of a request changes, and more of a bikeshedding moment where we need to decide how much we want to dig into being nativelink. My guess is a lot because of the value.

To run the cluster:

```bash
native up
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm strongly of the opinion that we should just call this nativelink. I know that gives you a lot of extra work, but I think it's better for recognition. native is just a bit too broad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming it is straightforward to rename it, but nativelink would interfere with the nativelink. If Pulumi supported rust we could theoretically create a version of nativelink that contains all of this logic behind a feature flag which could actually be a nice quality of life improvement, but it's currently not technically possible.

The trend with this CLI has been that it's gradually getting "slimmer" as more and more of the things this devcluster does is moved to Flux. As that trend continues it might be feasible to remove the entire thing at some point.

3. You should see an error message like:

```txt
error: hash mismatch in fixed-output derivation ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

:lgtm: Can discuss in another PR at another date when we have more time.

Reviewable status: 1 of 2 LGTMs obtained, and 0 of 3 files reviewed, and 2 discussions need to be resolved (waiting on @kubevalet)

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Dismissed @MarcusSorealheis from 2 discussions.
Reviewable status: 1 of 2 LGTMs obtained, and 0 of 3 files reviewed (waiting on @kubevalet)

To run the cluster:

```bash
native up
Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming it is straightforward to rename it, but nativelink would interfere with the nativelink. If Pulumi supported rust we could theoretically create a version of nativelink that contains all of this logic behind a feature flag which could actually be a nice quality of life improvement, but it's currently not technically possible.

The trend with this CLI has been that it's gradually getting "slimmer" as more and more of the things this devcluster does is moved to Flux. As that trend continues it might be feasible to remove the entire thing at some point.

3. You should see an error message like:

```txt
error: hash mismatch in fixed-output derivation ...
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

-@kubevalet

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained, and all files reviewed

@aaronmondal aaronmondal merged commit 4e3366d into TraceMachina:main Mar 18, 2025
37 checks passed
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