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

Install Ruby version inside actions job based on project #2755

Merged

Conversation

FloThinksPi
Copy link
Member

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

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.

  • An explanation of the use cases your change solves

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 branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

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.
@FloThinksPi
Copy link
Member Author

FloThinksPi commented Apr 5, 2022

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

@moleske
Copy link
Member

moleske commented Apr 5, 2022

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

Copy link
Member

@moleske moleske left a 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

.github/workflows/unit_tests.yml Show resolved Hide resolved
shell: bash
- uses: ruby/setup-ruby@v1
with:
bundler: "2.3.5"
Copy link
Member

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

Copy link
Member

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

  1. Prepackaging runs on your local machine, and bundles/vendors gems into a local cache
  2. 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 of bundle 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.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

@sethboyles sethboyles Apr 7, 2022

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

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).
@FloThinksPi FloThinksPi merged commit 2c5969c into cloudfoundry:main Apr 8, 2022
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.

None yet

4 participants