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

#692 : UrlUpdater fetches every defined version for Docker and latest version is now accepted as well #1117

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mbilda
Copy link
Contributor

@mbilda mbilda commented Mar 7, 2025

Fix: #692

  • deleted the new line patterns out of the regex in DockerDesktopUrlUpdater because Pattern.DOTALL already handles this
  • TBH I do not know why this pattern now works, because it also should have worked with the (\n\r...) regex. Maybe both, regex and DOTALL don't work together
  • changed the group to group1 because there is only one group left

With this implementation i was able to create versionzed folders in my custom repository for the defined docker versions:
{5977D301-C902-4C5E-B262-B6CA530FBE01}

This still needs to be tested with workflow because test <-> workflow isn't the same environment and so on.

In addition to the fix above i also added the fix for the actual latest problem.

  • Added a fallback for " * " (Latest) - Version to be found in the filesystem in AbstractUrlFolder. The getChild method looks for folders/files within the tool/edition. We're returning latest instead of " * " because "*" can not be found because theres actual no " * " in the folder hirachy but latest is. (latest is resolved to " * ")
  • Added a fallback to name the downloadcache file to latest if version is " * " (latest) in AbstractToolRepository. " * " is not a valid filename/foldername in windows. We are checking for " * " and using "latest" instead for the name.

Even tho the first fix should resolve the problem, because we do not have only latest-version in the url folder anymore, but in case there are other tools with only latest version it would be good to prevent the same error again.

We were also able to test the "real" UrlUpdater via github actions. We created a custom github action for testing the url updater. We checked out the url respository to fetch all existing versions and were able to upload/download the temporarly produced new repo.
The results can be seen here:
https://github.com/mbilda/IDEasy/actions/runs/13834518742
{47ACD727-B566-4E00-95F9-15F00A335179}

- deleted the new line patterns out of the regex in DockerDesktopUrlUpdater because Pattern.DOTALL already handles this
@mbilda mbilda requested a review from jan-vcapgemini March 7, 2025 14:33
@mbilda mbilda added bugfix docker docker and esp. DockerDesktop labels Mar 7, 2025
@coveralls
Copy link
Collaborator

coveralls commented Mar 10, 2025

Pull Request Test Coverage Report for Build 13903549158

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 70 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.1%) to 67.56%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/url/model/folder/AbstractUrlFolder.java 19 46.43%
com/devonfw/tools/ide/tool/repository/AbstractToolRepository.java 21 62.14%
com/devonfw/tools/ide/url/tool/docker/DockerDesktopUrlUpdater.java 30 0.0%
Totals Coverage Status
Change from base Build 13859584846: -0.1%
Covered Lines: 7827
Relevant Lines: 11157

💛 - Coveralls

jan-vcapgemini and others added 3 commits March 11, 2025 09:56
- Added a fallback for "*" (Latest) - Version to be found in the filesystem
- Added a fallback to name the downloadcache file to latest if version is "*" (latest)
@mbilda mbilda changed the title #692 : UrlUpdater fetches every defined version #692 : UrlUpdater fetches every defined version for Docker Mar 11, 2025
@mbilda mbilda changed the title #692 : UrlUpdater fetches every defined version for Docker #692 : UrlUpdater fetches every defined version for Docker and latest version is now accepted as well Mar 11, 2025
mbilda added 5 commits March 11, 2025 16:34
- Changed order to prevent nullpointer
- changed structure to if-else
- Refactored UrlUpdaterMockSingle to modify the verion which should be tested
- deleted temporary test case
@mbilda mbilda requested a review from hohwille March 13, 2025 13:08
@hohwille hohwille added this to the urls milestone Mar 14, 2025
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

@mbilda thank you very much for your PR. You did a great job and very well analysed the problem. Even you tested the URL Updater via GHA in your fork. Excellent 🚀

However, I checked the logs of your test run and found e.g. this line:

12:28:42.793 [com.devonfw.tools.ide.url.UpdateInitiator.main()] - WARN  - c.d.t.i.u.updater.AbstractUrlUpdater - For tool docker and version 4.33.2 the download verification failed with status code 403 for URL https://desktop.docker.com/win/main/amd64/179689/Docker%20Desktop%20Installer.exe.

It seems that there are a lot of such errors and I can also reproduce this in my browser:
image

My assumption is that after merging this we get dedicated versions of docker desktop but no URL for Windows and therefore we will entirely break it for Windows users.
Could you confirm my observations and maybe find a solution to improve your PR?

@hohwille
Copy link
Member

See also this final summary:

docker/docker versions added: 0 failed, 98 succeeded, 98 total, 0.00% error - versions verified: 17 failed, 56 succeeded, 73 total, 23.29% error

@mbilda
Copy link
Contributor Author

mbilda commented Mar 17, 2025

@hohwille good point.
So the error occurs because there is no version to download for windows at all. We still find the version because it is listed in the release notes, but there is no download link provided for windows - only for mac.
See here (example for 179689 - 4.33.2):
{5C2B37A7-0B6A-4159-93CF-8267C2480F8A}
That means there is no download link at all and thats why we receive this error, even when we directly pasting the download url into the browser.

Why do we even try to download this version?
The regex:

String regex = "href=#" + version
        // .....................................................(Group.1.)..........
        + ".{8,12}.{0,350}href=https://desktop\\.docker\\.com.*?(\\d{5,6}).*\\.exe";

The crucial part of this is the following:
.*\\.exe
It matches as many characters as needed until it finds .exe
The next match which will be found is actually the next version where a windows version exists.
See here (regex101 example matching two versions)
https://regex101.com/r/hEmRX3/2
At this point we do not know the "actual docker version" (179689) - thats why we use the regex above to extract it.
Even though it is working, we receive lots of errors with 403, because we try to download a non-existing file.

TODO:

  • We need a more specific regex to match exactly the stuff we need - not more, not less.
    • How could we do that?
      • We could delete the .exe of the regex to match the first finding of the docker-version-number. As we see in the above example, it doesn't matter if we have the .exe in the regex. It just searches for anything until it finds .exe - which will definitely be the case at some time. But at this point we already have what we want - the docker-version-number
      • We then could use the found group-match to execute a new regex but with specific windows urls or mac urls and so on (we actually do this in a different kind of way - we just try to download it - which is kinda finding it - but it fails)
      • After another regex, we can then download it, if it exists or not

Furthermore, if the download of the non-existing windows version fails, we return out of the addVersions()-function and skipping the "maybe existing" other versions for mac.

Questions:
Should we provide versions only for one OS at all?

  • Like in the example above, if we wouldn't return out of the function we would just download the 4.33.2 versions for mac and failing with windows. But then only mac-versions would exist in ide - good or bad?

@mbilda
Copy link
Contributor Author

mbilda commented Mar 17, 2025

The new implementation resolves the problem @hohwille mentioned. Lot of 403 errors appeared in our logs because there were no download links from docker at all for specific versions.
After getting the html body from docker release notes, we use this body to check if the download links for each version exist.
The new updated workflow shows only one error.
https://github.com/mbilda/IDEasy/actions/runs/13904290695
{237CB571-B2BE-4ECA-BED9-809FD12CFE76}
The one that failed is actually on the release site with download link, but also failing in browser => No issue with implementation.

Should we provide versions only for one OS at all?
Like in the example above, if we wouldn't return out of the function we would just download the 4.33.2 versions for mac and failing with windows. But then only mac-versions would exist in ide - good or bad?

Yes. We have other tools with the same behavior. Sometimes there are only versions for some operating systems. The admin needs to know that and handle this situation accordingly.

@mbilda mbilda requested a review from hohwille March 17, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix docker docker and esp. DockerDesktop
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

"Latest" version of Docker causes installation problems
4 participants