-
Notifications
You must be signed in to change notification settings - Fork 77
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
Fenix support #632
Fenix support #632
Conversation
d1a821a
to
f82782b
Compare
Hey @whimboo! Can you please take a look and suggest changes if any, thanks! |
@salilmishra23 I'm absolutely sorry. When you created the PR I was out and then too many notifications have been received that I'm still working through the list. Sadly it's not that quick. Now I have seen your PR and will make sure to review the code additions in a timely manner. I hope that you have still interest to continue in getting this features added to mozdownload! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting started to work on this issue! It's great to see the progress as already made! I added a couple of inline comments, so please have a look at these. Let me know if something is unclear and I'm happy to help. I'm looking really forward to a final version that we can merge. Thanks again!
Please note that due to recent changes you should rebase your branch against master. Otherwise tests in CI will fail. Thanks. |
98dfc54
to
ee197b8
Compare
Hey whimboo! I tried to address all the comments you had. Please let me know if you want any additional changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally had a look after coming back from absence and it's great to see that tests are passing. Sadly there are some further issues that need to be addressed given that right now I'm not able to download actual fenix builds from the remote server.
-
Please try to download various builds yourself. Like downloading a release build of Fenix via
mozdownload -a fenix -v latest -p android-x86_64
. It currently produces a wrong path so the build cannot be found underhttps://archive.mozilla.org/pub/fenix/releases/110.1.0/android/fenix-110.1.0-android-x86_64/
. As such we would need real remote tests similar to other applications. -
If Fennec is a problem I'm happy to just replace it's support with Fenix. Fennec is old and cannot be used anymore anyway. If someone needs it then an older version of mozdownload could be used.
Please let me know if you have questions.
40f42c6
to
795b4e4
Compare
991173c
to
f42a59a
Compare
Hey @whimboo, it would be great if you can review the PR again, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great update and I think by removing Fennec it made it much more simpler! Thanks for that work!
Please have a look at the inline comments that I've made. It's not major just some clean-ups and small fixes.
Beside that you should still have a look at the remote tests for Fenix. Those we definitely need to ensure our product is working as example:
- Please check the tests folder for left-over
mobile
traces, eg. remove thetests/data/mobile
folder - Fix the path for
mobile
tofenix
intest_api.py
. - Also a plus point if you could add unit tests for the release scraper for Fenix. Test files should be placed in
tests/data/fenix/releases
.
Once that's done there is nothing more left and the PR can be merged. Thanks again!
Hi @salilmishra23. I wanted to ask if you may have the time to get the remaining review comments addressed. I would like to release a new version of mozdownload soon and it would be great to have this feature included. Thanks! |
Hey @whimboo, sorry for the long delay. I have addressed all the comments, please have a look! |
Hi @salilmishra23 no worries. Great that you found the time to continue on this PR! I appreciate. Now that I'm back from days off due to holidays as well let me take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great now! All the variations of types, platforms work fine for Fenix. Thanks a lot!
There is just a small inline comment regarding missing tests. Let me know if you would be able to add these. Otherwise I can do myself after merging this PR.
Thanks for the reviewing! At the moment, I can't find time to add the required tests, please feel free to do the needful. |
Sorry for the delay. I think it's fine then when we move the creation of the additional tests to a follow-up issue / PR. I've filed #663 for it and I will take care of it. Thanks a lot for your contribution and the time you spent on learning about mozdownload. With the next release it will definitely help a lot of users in getting those builds! |
resolves #624