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

[Breaking] Rename cas executable to nativelink #573

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

aaronmondal
Copy link
Member

@aaronmondal aaronmondal commented Dec 21, 2023

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 +@allada

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @allada and @MarcusSorealheis)

@aaronmondal aaronmondal linked an issue Dec 21, 2023 that may be closed by this pull request
Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)

a discussion (no related file):
Since this is a breaking change, should we tally an entry into https://github.com/TraceMachina/nativelink/blob/main/CHANGELOG.md now or mark the issue tracker in a way such that next release it is easy to find what breaking changes have been made? Git commit titles work for me, maybe we should codify that (or alternatives) in https://github.com/TraceMachina/nativelink/blob/main/CONTRIBUTING.md

As for CHANGELOG.md format, do we need gitsha or just reference ticket id and traverse from there to commit? Personally I do like easy access to gitsha, but also seems like an additional thing to manage in terms of updating a changelog file.


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.

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), publish-image, ubuntu-20.04, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @allada and @MarcusSorealheis)

a discussion (no related file):

Previously, adam-singer (Adam Singer) wrote…

Since this is a breaking change, should we tally an entry into https://github.com/TraceMachina/nativelink/blob/main/CHANGELOG.md now or mark the issue tracker in a way such that next release it is easy to find what breaking changes have been made? Git commit titles work for me, maybe we should codify that (or alternatives) in https://github.com/TraceMachina/nativelink/blob/main/CONTRIBUTING.md

As for CHANGELOG.md format, do we need gitsha or just reference ticket id and traverse from there to commit? Personally I do like easy access to gitsha, but also seems like an additional thing to manage in terms of updating a changelog file.

I think that's a good thing in general but it's not clear to me what a good balance between keeping the changelog updated and small commits is.

Technically we only need to update the changelog on every tag. So we don't have to always keep it updated. I think I'm leaning more towards only bumping the changelog on commits that are intended to be tagged commits.


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.

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), publish-image, ubuntu-20.04, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @allada and @MarcusSorealheis)

a discussion (no related file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I think that's a good thing in general but it's not clear to me what a good balance between keeping the changelog updated and small commits is.

Technically we only need to update the changelog on every tag. So we don't have to always keep it updated. I think I'm leaning more towards only bumping the changelog on commits that are intended to be tagged commits.

Looking into this we'll probably have to make some changes to the existing changelog anyways. What we're currently doing is more or less a copy of the git history. That's explicitly not what we're supposed to do for the changelog. Instead, it should be a short, concise, human-readable thing that only contains relevant information on CVEs, migration paths and major changes.

See: https://github.com/coreinfrastructure/best-practices-badge/blob/main/docs/criteria.md#change-control


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.

@allada Is this safe to merge without breaking our existing deployments?

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04) (waiting on @allada 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.

Otherwise, looks good.

Cargo.toml Show resolved Hide resolved
@aaronmondal aaronmondal force-pushed the rename-cas-to-nativelink branch from 874a815 to c12456b Compare December 21, 2023 20:59
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.

Reviewable status: 0 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @adam-singer, @allada, @blakehatch, and @MarcusSorealheis)


Cargo.toml line 19 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

this change got me thinking, does this comment at the top of the file still hold? I feel these may have disappeared.

@allada do you know anything about that?

Done. This was indeed wrong.

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 4 LGTMs obtained (waiting on @adam-singer, @blakehatch, and @MarcusSorealheis)

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 a discussion.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@aaronmondal aaronmondal merged commit ddf1d74 into TraceMachina:main Dec 21, 2023
20 checks passed
Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

a discussion (no related file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Looking into this we'll probably have to make some changes to the existing changelog anyways. What we're currently doing is more or less a copy of the git history. That's explicitly not what we're supposed to do for the changelog. Instead, it should be a short, concise, human-readable thing that only contains relevant information on CVEs, migration paths and major changes.

See: https://github.com/coreinfrastructure/best-practices-badge/blob/main/docs/criteria.md#change-control

TIL on coreinfra github project


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.

Rename the Executable to nativelink
5 participants