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

Fix unity versions scrapping not getting all the versions #54

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

brako
Copy link
Contributor

@brako brako commented Oct 11, 2024

Changes

This pull request is fixing the unity versions scrapping.

The previous implementation was not returning all the versions anymore. The loop was stopping on the first non empty script tag containing at least one version. But currently it does not work.

The proposed fix is to loop on all the scripts tag, store all the versions in a map and returns them at the end.

It will probably fix the following issues: game-ci/docker#251, game-ci/docker#250

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@GabLeRoux GabLeRoux requested a review from AndrewKahr October 12, 2024 17:45
Copy link
Member

@GabLeRoux GabLeRoux left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I am unsure how this should be tested. The logic seems good:

  • The fix iterates through all script tags on the page.
  • It stores all found versions in a Map, ensuring all unique versions are collected.
  • Finally, it returns all unique versions, not stopping at the first found version.

This logic seems to be in line with the problem as explained in the PR description.

@GabLeRoux
Copy link
Member

GabLeRoux commented Oct 12, 2024

🚀 / Test Deploy (pull_request) Failing, this seems to be due to unavailalbe env var for this pull-request run (fork):

Either FIREBASE_TOKEN or GCP_SA_KEY is required to run commands with the firebase cli

Not sure if we should do something about it.

The tests are passing. I haven't seen any tests covering scrapeVersions method, might be worth testing it. I am personally ok with this as is because chances are this fix will unblock quite a few users trying to use recent unity versions with GameCI docker images.

@brako
Copy link
Contributor Author

brako commented Oct 12, 2024

I could add a test, but I'm unsure of the best way to implement it. Do you have any examples to share? We could check the number of versions returned by the scraping and determine that if it falls below a certain threshold, there's an issue. Currently, the scraping finds 576 versions.

As for the failing test, I'm not entirely familiar with the workflow, but it's possible that the failure is due to the pull request coming from a fork, which might be missing some GitHub secrets

Copy link
Member

@AndrewKahr AndrewKahr left a comment

Choose a reason for hiding this comment

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

Manually tested the code and the versions are now appearing to be scraped. Not sure how to make an automated test as the version count will always change so I think this can merge without one.

@AndrewKahr AndrewKahr merged commit fe20df7 into game-ci:main Oct 12, 2024
3 of 4 checks passed
@brako brako deleted the fix-unity-versions-scrapping branch October 12, 2024 22:19
@MrGadget1024
Copy link

** Thank You @brako !! **

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.

4 participants