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

Enforce "ruby" platform when fetching version data #8754

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

clemens
Copy link
Contributor

@clemens clemens commented Jun 12, 2024

When fetching version data for individual Ruby gems, we don't want to rely on creation order of the gems on RubyGems.org. Instead, we use the standard ruby platform as a sensible default (as we do elsewhere).

Partially addresses #8695.

@clemens clemens force-pushed the main branch 3 times, most recently from 5ccd02a to 7937b26 Compare June 12, 2024 09:04
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.79%. Comparing base (9e6bf29) to head (48c9ad7).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8754   +/-   ##
=========================================
  Coverage     67.79%   67.79%           
  Complexity     1164     1164           
=========================================
  Files           243      243           
  Lines          7711     7711           
  Branches        861      861           
=========================================
  Hits           5228     5228           
  Misses         2127     2127           
  Partials        356      356           
Flag Coverage Δ
funTest-non-docker 33.96% <ø> (ø)
test 38.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clemens
Copy link
Contributor Author

clemens commented Jun 12, 2024

@sschuberth @fviernau Thanks for the review comments. I had intentionally marked it as a Draft MR in order not to bother you to review yet, while I was sorting out the build etc. :) I guess my intention failed there.

Either way: It seems that there currently isn't a test that fails with the Gemfile changes aside from the obvious one which checks the resulting list.

My question here would be: If I revert all the changes to the Gemfile and Gemfile.lock and keep the change to the URL plus add a comment that references to the corresponding resolve_dependencies.rb change that it mimicks, would the change be acceptable for you even without an explicit test for it?

I just ran it in CI and here's what I'm getting:

Before the change:

Screenshot 2024-06-12 at 13 49 41

After the change:

Screenshot 2024-06-12 at 13 49 54

@sschuberth
Copy link
Member

I guess my intention failed there.

No worries. Sometimes we're just curious and willing to spend time on reviewing draft PRs, well knowing that our comments might become null and void due to ongoing changes.

would the change be acceptable for you even without an explicit test for it?

For me, it would be acceptable, as it's obviously doing the right thing, even if you cannot prove that it makes a difference according to test assets.

After the change:

Interesting that this writes out a smaller file although it has the same number of projects / packages. Mind investigating where the size difference comes from?

@clemens
Copy link
Contributor Author

clemens commented Jun 12, 2024

The main difference that I can spot is that the issues are captured in the analyzer-result.json:

Bundler Issues

There are 32 such entries. If I extract a single one and put it into a file, it's roughly 350-400 bytes (depending on the length of the respective project name) => 400 x 32 = ~12 KB. This comes fairly close to the (presumably rounded) 20 KB difference that the output lists.

@clemens clemens marked this pull request as ready for review June 12, 2024 14:14
@clemens clemens requested a review from a team as a code owner June 12, 2024 14:14
@clemens clemens force-pushed the main branch 4 times, most recently from dbf401c to 2356fa9 Compare June 13, 2024 13:16
@clemens
Copy link
Contributor Author

clemens commented Jun 14, 2024

So I adapted everything, but now we seem to have what looks like random/unrelated test failures…? I'm a bit stuck here. :/

@sschuberth
Copy link
Member

I'm a bit stuck here. :/

No worries. We'll realize this and eventually retrigger flaky tests etc.

@sschuberth
Copy link
Member

So I adapted everything

The commit message still talks about "we". Please reword as described above.

Fetching version data for individual Ruby gems shouldn't rely on
creation order of the gems on RubyGems.org. Instead, use the standard
`ruby` platform as a sensible default (as it already happens in
Bundler's resolving logic elsewhere in the code).

Partially addresses oss-review-toolkit#8695.

Signed-off-by: Clemens Kofler <[email protected]>
@sschuberth sschuberth dismissed fviernau’s stale review June 17, 2024 17:23

Comment addressed.

@sschuberth sschuberth enabled auto-merge (rebase) June 17, 2024 17:23
@sschuberth sschuberth merged commit 5e5296e into oss-review-toolkit:main Jun 17, 2024
20 checks passed
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.

3 participants