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

Add rust README.md #21

Merged
merged 14 commits into from
Oct 30, 2023
Merged

Add rust README.md #21

merged 14 commits into from
Oct 30, 2023

Conversation

idelvall
Copy link
Member

No description provided.

@idelvall idelvall requested a review from a team as a code owner October 30, 2023 19:52
rust/README.md Outdated Show resolved Hide resolved
Co-authored-by: Alex Couture-Beil <alex@earthly.dev>
Copy link
Member

@vladaionescu vladaionescu left a comment

Choose a reason for hiding this comment

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

Looks good overall! Left some comments regarding versioning...

rust/README.md Show resolved Hide resolved
rust/README.md Outdated
ARG --global debian = bookworm

# Importing via commit hash pinning because git tags can be changed
IMPORT github.com/earthly/lib/rust:4cfebf74b5805ad943d325a94601e808afbf6e6f AS rust
Copy link
Member

Choose a reason for hiding this comment

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

I know that tags can be changed, but changing them for released software is something you don't normally do. It might be good to start maintaining some official semver versions + tags for these subpackages. It would help users to keep track a bit better.

Perhaps the tags can be specific to a sub-package. e.g. rust-0.1.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

@vladaionescu why would we need semvers for specific packages and not for the whole repo?

Copy link
Member

Choose a reason for hiding this comment

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

Mainly because the compatibility guarantees might be better if they're isolated. Like if we break compatibility for rust, why have the go users worry about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but users can always look at the release notes, no? I just think it makes it harder (or at least eventually harder) to maintain

Copy link
Contributor

Choose a reason for hiding this comment

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

@vladaionescu I just feel like it's one of those things that can be addressed if and when it becomes a problem.

Copy link
Member Author

@idelvall idelvall Oct 30, 2023

Choose a reason for hiding this comment

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

Basically we would be breaking the semantics we are aiming to achieve. Is it truth that in a soft-way, but still we would be sending a confusing message:

  • Let's say I introduce a breaking change in rust, and increase the major version, then go users would perceive such version change and lose their stream of automatic updates
  • As an API user, it would be confusing and disappointing for me performing version updates that actually don't include any improvements. These changes just won't follow the semver specification

Copy link
Contributor

Choose a reason for hiding this comment

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

@idelvall , @vladaionescu
I suggest having a 5 minute discussion on this in our next core/work chats (or ad-hoc if we don't want to wait until then) before we commit to this.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good - booked!


First, import the UDC up in your Earthfile:
```earthfile
IMPORT github.com/earthly/lib/rust:<version/commit> AS rust
Copy link
Member

Choose a reason for hiding this comment

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

Nit: AS rust isn't actually necessary when the name is the same. But also it doesn't hurt.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is more readable

@idelvall idelvall merged commit ababd4a into main Oct 30, 2023
1 check passed
@idelvall idelvall deleted the nacho/rust-readme branch October 30, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants