-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Ruby 3.0 to CI matrix. #2459
Conversation
Ruby 3 is currently failing. Issue is related to: #2455. Fix has been pushed to master in http2: igrigorik/http-2#156 |
For the AR amongst us... JFYI, using:
The yaml is parsed as numbers, which is okay, but there are issues with trailing zeros in what appears to be a float, eg,
EDIT: I said 'which is okay'. Really, just ok for now. I think a numeric
|
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
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.
👍
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
@@ -17,6 +17,7 @@ Gem::Specification.new do |spec| | |||
spec.add_dependency('aws-partitions', '~> 1', '>= 1.239.0') # necessary for STS & S3 regional | |||
spec.add_dependency('aws-sigv4', '~> 1.1') # necessary for making Aws::STS API calls | |||
spec.add_dependency('aws-eventstream', '~> 1', '>= 1.0.2') # necessary for binary eventstream | |||
spec.add_dependency('rexml', '>= 0') |
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.
Talked offline about this - I think instead, customers should depend on any XML parser that we support (bring your own parser), and we should not force dependencies that may not be necessary and to avoid potential breakage. In general we try to own and control our own dependencies.
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 agree with this. The consequence however is that customers will need to specify (directly or indirectly) an xml library in order to use the SDK. In many cases rexml or another library will be brought in by other libraries anyway so this may not impact many users. However, I will add descriptive error messages - currently we assume that there will always be at least one xml engine available and dont handle the missing case (since rexml was previously always available).
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.
Is there potential for reversing this decision? It seems odd to have an undeclared dependency, that wouldn't be discovered until runtime, and only when using Ruby 3+.
Given that Ruby 3 still includes rexml
as a bundled gem, it should generally be available everywhere anyhow.
IMO this is a breaking change that is a bit odd for minor version bump.
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.
Adding it as an explicit dependency breaks older rubies, which we must still support. It's also not a required dependency: we support a number of xml engines, and will use whichever a client has loaded. I don't believe that not including a new dependency is a breaking change for existing users - it might, in a few cases, require changes in a users code when they are upgrading from an older ruby version to 3.
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459) - Expand pipeline to test using multiple Ruby versions
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459) - Expand pipeline to test using multiple Ruby versions
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459) - Expand pipeline to test using multiple Ruby versions
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the
CHANGELOG.md
file (at corresponding gem). For the description entry, please make sure it lives in one line and starts withFeature
orIssue
in the correct format.For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/master/CONTRIBUTING.md
Thank you for your contribution!