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-toolchain file and bash script to update Docker, Travis and rustup-toolchain files with same version #212

Merged
merged 1 commit into from Mar 29, 2019

Conversation

jxs
Copy link
Contributor

@jxs jxs commented Mar 14, 2019

closes linkerd/2482

Travis file is using the stable version, is there a special reason or can just be updated to the matching versions from Dockerfile and rustup-toolchain?

thanks 🙂

@jxs jxs force-pushed the master branch 2 times, most recently from 315641f to fe3628d Compare March 14, 2019 16:57
@jxs jxs changed the title add rustup-toolchain file and bash script to update Docker, Travis and rustup-toolchain files with same version add rust-toolchain file and bash script to update Docker, Travis and rustup-toolchain files with same version Mar 14, 2019
@@ -0,0 +1,5 @@
VERSION=$1
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should probably have a #!/bin/bash shebang?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes totally forgot it 😅

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Hey @jxs, thanks a lot!

Travis file is using the stable version, is there a special reason or can just be updated to the matching versions from Dockerfile and rustup-toolchain?

It's probably best for Travis to be using the same version as the Dockerfile , so I think you made the right call.

This change looks good to me, but I suspect that @olix0r will have opinions about bash scripts, so I'd like to get his approval before merging it.

.travis.yml Outdated
@@ -12,7 +12,7 @@ branches:
jobs:
include:
# Quickly run tests in dev mode.
- rust: stable
- rust: 1.32.0
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here that this should be kept in sync with the rust-toolchain file and the Dockerfile? It could say something like "rather than updating this manually, run update-rust-version.sh...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added :) also added to the Dockerfile

rust-toolchain Outdated Show resolved Hide resolved
@hawkw hawkw requested a review from olix0r March 14, 2019 17:18
@jxs jxs force-pushed the master branch 4 times, most recently from bea4f60 to f351292 Compare March 16, 2019 18:27
@olix0r
Copy link
Member

olix0r commented Mar 26, 2019

It's probably best for Travis to be using the same version as the Dockerfile , so I think you made the right call.

I think I disagree with this... I'd rather that we test and publish from the latest stable so that we discover issues eagerly, and also benefit from fixes. There may be a point in the future we we'd prefer to have more process around version updates, but it feels like a step backwards now.

@jxs
Copy link
Contributor Author

jxs commented Mar 26, 2019

@olix0r so, want me to update the bash script and remove the travis.yml file update?

rust-toolchain Outdated
@@ -0,0 +1 @@
1.32.0
Copy link
Member

Choose a reason for hiding this comment

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

1.33.0 is out now ;)

@olix0r
Copy link
Member

olix0r commented Mar 26, 2019

@jxs Yeah, let's do that for now, and we can add that back later if it seems important.

@hawkw
Copy link
Member

hawkw commented Mar 26, 2019

It's probably best for Travis to be using the same version as the Dockerfile , so I think you made the right call.

I think I disagree with this... I'd rather that we test and publish from the latest stable so that we discover issues eagerly, and also benefit from fixes. There may be a point in the future we we'd prefer to have more process around version updates, but it feels like a step backwards now.

@olix0r What about having a travis build against the latest stable and one against the version built in the Dockerfile? We could use the latest stable build to be aware of upcoming breaking changes, and the pinned version build to verify that the dockerfile will actually build a working proxy image.

@olix0r
Copy link
Member

olix0r commented Mar 26, 2019

@hawkw and I discussed this in realtime... I'll summarize:

The problem we care most about addressing is when a new developer is (1) running a very old rust version, so they cannot compile a recent proxy or (2, more likely) the user has a nightly that supports features we don't accept in our build.

For this case, we want a rust-toolchain to force users in case (1) to a newer minimum rust version and we want to force users in case (2) to use a stable rust.

To this end, I think it's okay if we continue to use stable in CI so that release artifacts are published from the most-recent Rust stable. We don't expect that newer rusts in CI will break existing code, so this is okay.

Then, we can manually bump rust-toolchain versions with the script when we take a new minimum version for development.

In summary: let's leave the travis.yml as it was (on master).

@jxs jxs force-pushed the master branch 3 times, most recently from 4d2237c to b476648 Compare March 27, 2019 01:03
@jxs
Copy link
Contributor Author

jxs commented Mar 27, 2019

thanks for the feedback loop
updated!

…rust-toolchain files with same version

Signed-off-by: João Oliveira <hello@jxs.pt>
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

thank you @jxs !

@olix0r olix0r merged commit e330c29 into linkerd:master Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants