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 CI step to release workflow to check for semver hazards #3383

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

jdisanti
Copy link
Collaborator

This CI step will check for subtle semver hazards with the ConfigBag during release. Once all the runtime crates are independent, then this check can be moved into normal CI.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-semver-hazard-ci branch 3 times, most recently from 10e1700 to 4c41dcc Compare January 24, 2024 19:29
@jdisanti jdisanti force-pushed the jdisanti-semver-hazard-ci branch from 4c41dcc to cfa45d6 Compare January 24, 2024 19:50
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@jdisanti
Copy link
Collaborator Author

Ran a dry-run against this branch to confirm the new check works: https://github.com/smithy-lang/smithy-rs/actions/runs/7645480204/job/20832185070

@jdisanti jdisanti marked this pull request as ready for review January 24, 2024 21:15
@jdisanti jdisanti requested review from a team as code owners January 24, 2024 21:15
#

# This script patches the new runtime crates into an old AWS SDK and runs tests
# to check for semver hazards, such as a `Storable` being in an unstable runtime crate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should note how to set this up (notably, $PWD/aws-sdk-rust, and smithy-rs need to exist)

Comment on lines +36 to +41
for sdk in dynamodb s3; do
echo -e "${C_YELLOW}# Testing ${sdk}...${C_RESET}"
pushd "aws-sdk-rust/sdk/${sdk}" &>/dev/null
cargo test --all-features
popd &>/dev/null
done
Copy link
Collaborator

@rcoh rcoh Feb 6, 2024

Choose a reason for hiding this comment

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

should we cargo check a few more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think cargo check will do a whole lot for us here since these breaks happen at runtime, unless you're thinking of having it detect backwards compatibility issues. These two services should compile enough runtime crates that I think it would catch most of those.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

have you tried creating an intentional semver break and then running release in preview mode?

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

have you tried creating an intentional semver break and then running release in preview mode?

@jdisanti
Copy link
Collaborator Author

jdisanti commented Feb 8, 2024

have you tried creating an intentional semver break and then running release in preview mode?

I verified that locally by invoking the script manually. I also confirmed the script is working correctly in GitHub Actions in https://github.com/smithy-lang/smithy-rs/actions/runs/7645370946/job/20831801559, where you can see it correctly picked up the patched crate version and then failed to compile due to deprecations and -D warnings (the latter of which I removed in a later revision of this PR):

   Compiling aws-sdk-s3 v1.13.0 (/home/build/workspace/aws-sdk-rust/sdk/s3)
error: use of deprecated type alias `aws_http::user_agent::AwsUserAgent`: Use aws_runtime::user_agent::AwsUserAgent instead.
 --> sdk/s3/tests/request_information_headers.rs:6:27
  |
6 | use aws_http::user_agent::AwsUserAgent;
  |                           ^^^^^^^^^^^^
  |
  = note: `-D deprecated` implied by `-D warnings`

error: use of deprecated type alias `aws_http::user_agent::AwsUserAgent`: Use aws_runtime::user_agent::AwsUserAgent instead.
  --> sdk/s3/tests/request_information_headers.rs:50:29
   |
50 |             layer.store_put(AwsUserAgent::for_tests());
   |                             ^^^^^^^^^^^^

error: use of deprecated constant `aws_http::content_encoding::header_value::AWS_CHUNKED`: Use aws_runtime::content_encoding::header_value::AWS_CHUNKED instead.
   --> sdk/s3/tests/checksums.rs:229:76
    |
229 |         HeaderValue::from_static(aws_http::content_encoding::header_value::AWS_CHUNKED),
    |                                                                            ^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

@jdisanti jdisanti added this pull request to the merge queue Feb 9, 2024
Merged via the queue into main with commit 0be5cb3 Feb 9, 2024
55 of 57 checks passed
@jdisanti jdisanti deleted the jdisanti-semver-hazard-ci branch February 9, 2024 00:42
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