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

ci: fix ignored errors within appraisal loop #1103

Merged
merged 7 commits into from
Aug 8, 2024

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Aug 6, 2024

The existing loop over appraisals for a gem is swallowing any error exits returned by commands within the loop.

Resolves #1097. The specific error mentioned in that issue for failure to bundle install pg 1.2 was fixed in #1108. This change fixes the appraisal loop.

Fix appraisal loop swallowing errors:

  • add an explicit exit to the appraisal loop if any command in the chain fails so that the CI step also fails

Fix other errors that were being swallowed:

  • add a workaround for GEM_HOME file permission problems during bundle install
    • introduced to GitHub runner images sometime early July 2024
  • bound which version of Ruby the gruf gem tests are run against
    • its gemspec includes upper bounds on Ruby versions which cause bundler install to fail under newer Rubies

@robbkidd
Copy link
Member Author

robbkidd commented Aug 6, 2024

This has unearthed filesystem permissions set in the pre-built runners that bundler does not approve of. There is an issue open about world-writtable Ruby install location. I'll add a workaround in the meantime.

Remove write access to the world on GEM_HOME. Hopefully a temporary
workaround until the runner image is fixed.

See actions/runner-images#10215
@robbkidd robbkidd force-pushed the whats-up-with-appraising branch from a0899fa to 4ac28e3 Compare August 7, 2024 21:05
gruf releases with lower/upper limits for spec.required_ruby_version.
So we won't test under Ruby versions that a gruf version won't
successfully resolve for.
@robbkidd robbkidd changed the title fix: 🚧 investigating ignored errors in appraisal loop ci: fix ignored errors within appraisal loop Aug 7, 2024
@robbkidd robbkidd marked this pull request as ready for review August 7, 2024 23:04
@robbkidd robbkidd requested review from a team August 7, 2024 23:04
@robbkidd
Copy link
Member Author

robbkidd commented Aug 7, 2024

It might make sense to fix the pg tests in a separate PR depending on how we'd like to communicate the drop support. This PR so far is a non-breaking change.

@@ -100,7 +107,7 @@ runs:
echo "::group::🔎 Appraising ${i}"
BUNDLE_GEMFILE=gemfiles/${i}.gemfile bundle install --quiet --jobs=3 --retry=4 && \
BUNDLE_GEMFILE=gemfiles/${i}.gemfile bundle show && \
BUNDLE_GEMFILE=gemfiles/${i}.gemfile bundle exec rake test
BUNDLE_GEMFILE=gemfiles/${i}.gemfile bundle exec rake test || exit
Copy link
Collaborator

Choose a reason for hiding this comment

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

would using bundle install --path using a relative vendor directory address the permissions issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably! I cannot confirm a fix locally, so I'll throw another commit on here changing the workaround from the separate chmod step to including a --path switch here. Then I'll push that up and see how CI reacts.

Copy link
Member Author

Choose a reason for hiding this comment

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

bundle install --path is deprecated, so I went with bundle config path in d7ba5f4. Now watching CI. ⌛

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. bundle config path didn't work for bundler's filesystem permission check. I'm going to revert that commit and go back to chmod.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@simi any tips here?

@kaylareopelle
Copy link
Contributor

@robbkidd - Thanks for putting this together! It looks like the issue about the versions of pg we're testing will be fixed in #1106

Alternative workaround to /opt/hostedtoolcache/Ruby being world writable
in the ubuntu 20240708.1.0 GA runner image.

See actions/runner-images#10215
@robbkidd
Copy link
Member Author

robbkidd commented Aug 8, 2024

@kaylareopelle, to get CI on main passing appropriately and soon, I am probably going to open a new PR for the change to the pg Appraisals. @matthewtusker said it's going to take some time to navigate the CLA signing process with his company and it would be fine with him for someone else to submit the change.

This reverts commit d7ba5f4.

'bundle config path' was not enough to workaround bundler's
filesystem permission check.
@robbkidd robbkidd merged commit 3584cfa into open-telemetry:main Aug 8, 2024
59 checks passed
@robbkidd robbkidd deleted the whats-up-with-appraising branch August 8, 2024 19:20
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.

Can't run tests in Docker Containers
3 participants