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

Bump ruby version to ruby 3 #2703

Merged
merged 3 commits into from May 6, 2022
Merged

Bump ruby version to ruby 3 #2703

merged 3 commits into from May 6, 2022

Conversation

sethboyles
Copy link
Member

[#2619]

Co-authored-by: Seth Boyles sboyles@pivotal.io
Co-authored-by: Alex Rocha alexr1@vmware.com

@sethboyles
Copy link
Member Author

This supercedes #2623

[#2619]

Co-authored-by: Seth Boyles <sboyles@pivotal.io>
Co-authored-by: Alex Rocha <alexr1@vmware.com>
@sethboyles sethboyles marked this pull request as ready for review March 3, 2022 21:49
@Gerg Gerg self-requested a review March 3, 2022 22:33
- missed it in the other commit

Authored-by: Michael Oleske <moleske@pivotal.io>
@FloThinksPi FloThinksPi self-requested a review March 23, 2022 09:36
Copy link
Member

@FloThinksPi FloThinksPi left a comment

Choose a reason for hiding this comment

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

Based on the unit test github actions workflow i guess the unittests did not actually run with neither ruby 3.0.3 nor 2.7.2 but with ruby 2.7.5 https://hub.docker.com/layers/capi/cloudfoundry/capi/ruby-units/images/sha256-42cb7efe67a2354d1456f54d218fdfe4e7185de69271a89d6511645444c2fbf3?context=explore

I would propose to use a more generic base image and install the used ruby version based on the .ruby-versions file dynamically in the action job (like its done with ruby gems already)

@sethboyles
Copy link
Member Author

sethboyles commented Mar 23, 2022

Yeah, we have a PR to update the dockerfiles: https://github.com/cloudfoundry/capi-dockerfiles/pull/18/files

Installing the ruby version based on the .ruby-version file is a good idea, but I wonder how much time that would add to our test runs.

@moleske
Copy link
Member

moleske commented Mar 23, 2022

The mysql8 checks ran with ruby3 per the ruby setup job

this is because the mysql8 github action does not use the docker image and instead runs directly on the github provided ubuntu machine. It also reads from ruby version file to determine the version of ruby to use. It probably wouldn't be too bad to switch the rest of the github actions to run like the mysql8 one, but does open the question that it would be different than how concourse runs the tests

@FloThinksPi
Copy link
Member

FloThinksPi commented Mar 24, 2022

Yea that uses the setup-ruby action which likely just downloads a binary as it just adds 4s. Would love to see that and then also no additional image may be required since https://github.com/cloudfoundry/capi-dockerfiles/blob/b8b3a8702f645a96861fac4ab8a72d7c1b0d0f15/capi-runtime-ci/Dockerfile#L10-L32 should also just add a few seconds to the job. Would make the CI setup a lot easier and flexible.

One question to the ruby update itself, why is the large amount of adoptions to brackets and double splatter operators of the WIP PR not needed anymore here ?
https://github.com/cloudfoundry/cloud_controller_ng/pull/2623/files#diff-8c147d0121d57d3714f83453ae1f79b5e943700ae6825b7912659750729d15adL126

@sethboyles
Copy link
Member Author

but does open the question that it would be different than how concourse runs the tests

The original idea was to use the OCI image that we use in concourse, so we aren't surprised by failures occurring upon merging PRs. It's probably enough just to run units for now, but it would also be nice to figure out how to get a similar change in the concourse pipeline

One question to the ruby update itself, why is the large amount of adoptions to brackets and double splatter operators of the WIP PR not needed anymore here ?

Those were merged into main ahead of time: #2651. There were a few other PRs breaking out other changes, too.

@tjvman tjvman mentioned this pull request Mar 29, 2022
12 tasks
@MMisoch
Copy link
Contributor

MMisoch commented Mar 31, 2022

Hi, I run the CATs with the changes introduced in this PR on our Azure dev-landscape and they ran through just fine. I run the version v7.3.0 of the CATs which is the latest release. I patched the changes into capi-release 1.127.0. I run the test twice as the first one got some flakey errors which where gone the second run. Tests took about 2 hours and I basically used the example-CATs-config. We could not find any anomalies in blobstore operations in particular as this seems to be the most interesting part here.
We will now continue on our Alicloud dev-landscape as soon as possible and give feedback on those as well.

@MMisoch
Copy link
Contributor

MMisoch commented Apr 8, 2022

Hi all,
CATS on the Alicloud Dev-landscape are done. Once again I run v7.3.0 of the CATs. I patched the changes into capi-release 1.127.0 and still used the example-CATs-config basically. There were no noticeable problems introduced with Ruby 3.
We will now continue on our AWS and GCP dev-landscape and update the PR again soon.

Best Regards

@sethboyles
Copy link
Member Author

@MMisoch thanks for the update. Should we just merge? We have GCP and AWS blobstores in the main CAPI pipeline, so those will get tested regardless. We can revert if there are particular issues.

@MMisoch
Copy link
Contributor

MMisoch commented Apr 21, 2022

Hi all,
CATS on the GCP Dev-landscape are done. This time I used v7.4.0 of the CATs. I patched the changes into capi-release 1.127.0 once again.
On GCP there were also no noticeable problems.
@sethboyles I think we should go ahead and merge as you suggested.

Best Regards

@MMisoch
Copy link
Contributor

MMisoch commented Apr 21, 2022

We had a short internal discussion and were wondering if it would be possible to cut a release that only contains the ruby 3 changes and not other changes as well.
What is your opinion about that?

Best regards

@sethboyles
Copy link
Member Author

That sounds fine to us. Do you all mind orchestrating that?

@MMisoch
Copy link
Contributor

MMisoch commented Apr 22, 2022

We could do that no problem. We would just need to wait for the next release and do the ruby3 one on top of that.

@sethboyles
Copy link
Member Author

@MMisoch just wondering, did you all test to see that similar issues to #2748 didn't arise when deploying?

@philippthun philippthun merged commit b62ade4 into main May 6, 2022
@philippthun philippthun deleted the bump-ruby branch May 6, 2022 10:52
MerricdeLauney pushed a commit that referenced this pull request May 9, 2022
Reverting for now. Breaking pipelines

This reverts commit b62ade4, reversing
changes made to 3505845.
moleske added a commit that referenced this pull request May 31, 2022
moleske added a commit that referenced this pull request Jun 23, 2022
moleske added a commit that referenced this pull request Jul 1, 2022
moleske added a commit that referenced this pull request Jul 6, 2022
* Revert "Revert "Merge pull request #2703 from cloudfoundry/bump-ruby""

This reverts commit 4971a57.

* Fixing ruby3 scheduler errors stemming from invalid ruby3 syntax in method call

Co-authored-by: David Alvarado <alvaradoda@vmware.com>
Co-authored-by: Alex Rocha <alexr1@vmware.com>

* bump to ruby 3.0.4

Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: David Alvarado <alvaradoda@vmware.com>

* Update specs that had mismatched keyword args/hash opts

* we found these by running the latest branches of rspec--the currently
  released versions miss these when they are mismatched

Co-authored-by: Seth Boyles <sboyles@pivotal.io>
Co-authored-by: Michael Oleske <moleske@pivotal.io>

Co-authored-by: M. Oleske <michael@oleske.engineer>
Co-authored-by: David Alvarado <alvaradoda@vmware.com>
Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: Seth Boyles <sboyles@pivotal.io>
will-gant pushed a commit to sap-contributions/cloud_controller_ng that referenced this pull request Dec 16, 2022
…uby"

Reverting for now. Breaking pipelines

This reverts commit b62ade4, reversing
changes made to 3505845.
will-gant pushed a commit to sap-contributions/cloud_controller_ng that referenced this pull request Dec 16, 2022
* Revert "Revert "Merge pull request cloudfoundry#2703 from cloudfoundry/bump-ruby""

This reverts commit 4971a57.

* Fixing ruby3 scheduler errors stemming from invalid ruby3 syntax in method call

Co-authored-by: David Alvarado <alvaradoda@vmware.com>
Co-authored-by: Alex Rocha <alexr1@vmware.com>

* bump to ruby 3.0.4

Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: David Alvarado <alvaradoda@vmware.com>

* Update specs that had mismatched keyword args/hash opts

* we found these by running the latest branches of rspec--the currently
  released versions miss these when they are mismatched

Co-authored-by: Seth Boyles <sboyles@pivotal.io>
Co-authored-by: Michael Oleske <moleske@pivotal.io>

Co-authored-by: M. Oleske <michael@oleske.engineer>
Co-authored-by: David Alvarado <alvaradoda@vmware.com>
Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: Seth Boyles <sboyles@pivotal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants