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 Ruby 3.0 to CI matrix. #2459

Merged
merged 10 commits into from
Jan 6, 2021
Merged

Add Ruby 3.0 to CI matrix. #2459

merged 10 commits into from
Jan 6, 2021

Conversation

alextwoods
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

  1. 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 with Feature or Issue in the correct format.

  2. 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!

@alextwoods
Copy link
Contributor Author

Ruby 3 is currently failing. Issue is related to: #2455.

Fix has been pushed to master in http2: igrigorik/http-2#156

@MSP-Greg
Copy link

MSP-Greg commented Jan 4, 2021

For the AR amongst us...

JFYI, using:

ruby: [2.3, 2.4, 2.5, 2.6, 2.7, 3.0, jruby-9.2]

The yaml is parsed as numbers, which is okay, but there are issues with trailing zeros in what appears to be a float, eg, 2.2.10 would be okay, but 3.0 or 2.10 would not. Hence, using the following shows 3.0 in the jobs list:

ruby: [2.3, 2.4, 2.5, 2.6, 2.7, '3.0', jruby-9.2]

EDIT: I said 'which is okay'. Really, just ok for now. I think a numeric 3.0 would resolve to the highest/latest release of '3', so it would be an issue when 3.1 is released, eg, using the following would result in two jobs, both using 3.1:

ruby: [2.3, 2.4, 2.5, 2.6, 2.7, 3.0, 3.1, jruby-9.2]

@github-actions
Copy link

github-actions bot commented Jan 4, 2021

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2021

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

Copy link

@igrigorik igrigorik left a comment

Choose a reason for hiding this comment

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

👍

@github-actions
Copy link

github-actions bot commented Jan 6, 2021

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

@github-actions
Copy link

github-actions bot commented Jan 6, 2021

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

@github-actions
Copy link

github-actions bot commented Jan 6, 2021

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')
Copy link
Contributor

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.

Copy link
Contributor Author

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).

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.

Copy link
Contributor Author

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.

@github-actions
Copy link

github-actions bot commented Jan 6, 2021

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

@alextwoods alextwoods merged commit cc06703 into master Jan 6, 2021
@github-actions
Copy link

github-actions bot commented Jan 6, 2021

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

@mullermp mullermp deleted the ruby3-ci branch January 6, 2021 23:13
arothian added a commit to arothian/cfn_nag that referenced this pull request Jan 20, 2021
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459)
- Expand pipeline to test using multiple Ruby versions
arothian added a commit to stelligent/cfn_nag that referenced this pull request Jan 29, 2021
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459)
- Expand pipeline to test using multiple Ruby versions
sfo-den pushed a commit to sfo-den/stelligent that referenced this pull request Oct 7, 2024
- Add rexml as explicit dependency (aws/aws-sdk-ruby#2459)
- Expand pipeline to test using multiple Ruby versions
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.

5 participants