-
Notifications
You must be signed in to change notification settings - Fork 362
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
Install Ruby version inside actions job based on project #2755
Install Ruby version inside actions job based on project #2755
Conversation
With this change one does not need to maintain own OCI images for github actions. Each run is based on a stock ubuntu image which is setup acordingly at runtime of a job. This makes it possible to run the unit tests with exactly what is defined in cloud_controller_ng and not have to sync a OCI image to the state of the cloud_controller_ng project.
@moleske got it working now :). Btw did you enable actions beeing executed from forks ? It is running now in this repo but i am not sure if e.g. Secrets are passed to the jobs(and thats a reason why they should not be executed by default). If that would be the case any person doing a fork and pr could dump the secrets. However since i am not an admin i cannot check the actions config in this repo :). |
I think we have enabled github actions from "trustworthy" users. People who have certain administrative rights to a repository can allow certain github users PRs from that user's fork to run github actions. Though maybe it is per an org since I see the PR is from https://github.com/sap-contributions and not your personal fork edit - I looked in settings of the repo. We require someone with admin access to approve first time contributors. So a PR won't run github actions from someone who has not contributed before, but someone with administrative access to the repo can approve that PR from the first timer. Then that first timer doesn't need repeat approval. See https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#configuring-required-approval-for-workflows-from-public-forks for more |
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.
Functionally looks good to me, just small comment about how concerned we should be about managing a bundler version outside of the gemfile. Curious to hear from folks who've been working on cloud controller longer than I about any concerns with github actions being very different setup wise from the concourse pipeline
shell: bash | ||
- uses: ruby/setup-ruby@v1 | ||
with: | ||
bundler: "2.3.5" |
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.
little surprised to see specific bundler version. When we were working on ruby 3 upgrade, we went with matching the bundler version that comes with ruby 3 so we didn't have install/specify a special version. Probably not a big deal though, just somewhere else we need to keep bundler in sync in addition to the gemfile
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.
Let me see if I can recall this correctly...
Packaging has two steps prepackaging and packaging
- Prepackaging runs on your local machine, and bundles/vendors gems into a local cache
- Packaging builds CCNG on the BOSH vm, using the cached directory created in prepackaging. This is done by calling
bosh_bundle_local
, which is defined here. It basically does a version ofbundle install --local
, which is described as thus:
--local
Do not attempt to connect to rubygems.org. Instead, Bundler will use the gems already present in Rubygems' cache or in vendor/cache. Note that if an appropriate platform-specific gem exists on rubygems.org it will not be found.
Packaging, because it runs on the VM (and it needs to be able to run without internet), generally only has access to the version of bundler packaged with the BOSH Ruby release..
So prepackaging with a Bundler version newer than the Bundler version the VM has access to can run into issues. Normally you get a warning when bundling with an older version, but it's generally OK--but bundling against a local cache made with a newer version of Bundler can be more error prone. It's been awhile, but I think we had this issue with the newrelic plugin gem. More details here: https://www.pivotaltracker.com/n/projects/966314/stories/176467695
So I wouldn't recommend bumping the BUNDLED WITH
beyond what the bosh release has.
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 an aside, It's interesting that information about prepackaging has been made intentionally hard to find. In the bosh.io packaging tutorial, they say this:
Use of the pre_packaging file is not recommended, and is not discussed in this tutorial.
Instead they package gems manually. For us, would that mean committing the vendor directory to CCNG? Not that it would resolve this issue with mismatching Bundler versions
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.
@sethboyles @moleske Yea so the issue is there is a bug in bundler that lets installs fail on the used ubuntu image and runner combination https://github.com/sap-contributions/cloud_controller_ng/runs/5864281143?check_suite_focus=true
rubyjs/mini_racer#218
One needs bundler 2.3 for this to work. But it should be enough to increase the bundler version just in the docs/v3/gemfile.lock and not in the main gemfile.lock. Would this be fine then?
Added a commit to handle bundler versions properly according to whats in the gemfile.lock
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.
Yeah, I think it's OK for the docs gemfile.lock
2e8e343
to
d6fa63c
Compare
Skips a initial step where it would build a binary for the slack reporter in the beginning of each job. Now it is just build and executed when really needing to use it(report a failure).
d6fa63c
to
60f3b04
Compare
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
With this change one does not need to maintain own OCI images for github
actions. Each run is based on a stock ubuntu image which is setup
acordingly at runtime of a job. This makes it possible to run the unit
tests with exactly what is defined in cloud_controller_ng and not have
to sync a OCI image to the state of the cloud_controller_ng project.
Simplifies Github action setup.
Reuses the same part of steps to set up an image.
Makes building an own OCI image for CI obsolete.
Installs the ruby version from the code so ruby version bumps are correctly tested.
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
main
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests