-
Notifications
You must be signed in to change notification settings - Fork 222
Generate a versions.toml with the SDK
#1311
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
Conversation
rcoh
left a comment
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.
as discussed offline:
- simplify file listing logic
Otherwise, seems good!
.github/workflows/ci.yml
Outdated
| path: smithy-rs-base-image | ||
| retention-days: 1 | ||
|
|
||
| # Run the PR bot after acquiring the build Docker image is completed since it will use it. |
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.
very hard to parse this sentence
|
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
|
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
| action: | ||
| description: What action to run in the Docker build | ||
| required: true | ||
| action-arguments: |
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.
maybe just arguments? up to you
| "--smithy-build", | ||
| outputDir.resolve("../../smithy-build.json").normalize().absolutePath, | ||
| "--examples-revision", | ||
| properties.get("aws.sdk.examples.revision") ?: "missing" |
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.
will this be confusing when it happens? should we hard fail if this is missing?
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.
It's definitely not ideal, but the alternative means not being able to run ./gradlew aws:sdk:assemble locally without checking out a copy of aws-doc-sdk-examples
| rustup component add rustfmt; \ | ||
| rustup component add clippy; \ | ||
| rustup install ${rust_nightly_version}; \ | ||
| rustup toolchain install ${rust_nightly_version} --component clippy; \ |
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.
side note: we might want to be explicit about the toolchains we install. I've run into some surprises from time to time relying on the defaults (and this might also install components we don't need)
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.
https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file may also be handy
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.
I think the --profile minimal on rustup-init is making sure it won't install any undesired components.
| set -eux | ||
|
|
||
| echo -e "${C_YELLOW}Taking examples from 'awsdocs/aws-doc-sdk-examples'...${C_RESET}" | ||
| examples_revision=$(cd aws-doc-sdk-examples; git rev-parse HEAD) |
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.
shellcheck would probably want quotes here?
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.
Surprisingly not in this case...
| } | ||
|
|
||
| let mut model_hash = None; | ||
| if package.handle.name.starts_with("aws-sdk-") { |
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.
how is this different than PackageCategory::AwsSdk?
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.
It isn't. Not sure why I did it that way. Will switch to using the category.
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
Motivation and Context
This is prerequisite work for tracking SDK crate versions independently, as described in the RFC. This PR:
crate-hashertool that builds against Cargo and uses the same code paths that find files for thecargo packagesubcommand, and then SHA-256 hashes those files in a deterministic waygenerate-version-manifestsubcommand in thepublishertool which examines the generated SDK and runs thecrate-hasheragainst it in order to produce aversions.tomlfilegenerate-version-manifestsubcommand into the Gradleaws:sdk:assembletask so that the generated SDK now has aversions.tomlmanifestBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.