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(environment): Clone IDF release branches instead of downloading zip and then fastforward #274

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

Conversation

jakub-kocka
Copy link
Collaborator

@jakub-kocka jakub-kocka commented Sep 11, 2024

When the selected IDF branch is the release one, perform git clone instead of downloading a zip archive and fast-forwarding to the latest commit

Releated

  • based on the Mattermost discussion (mention in tracker)
  • internal tracker: IDF-10228

Tested

  • online IDF Installer (GitHub, Gitee and JihuLab mirrors)
    • release branch (zip archive correctly NOT downloaded -> git clone performed)
    • master branch (zip archive correctly NOT downloaded -> git clone performed)
    • released IDF version (zip archive correctly downloaded)
  • offline IDF Installer (working as usual without any issues)

@jakub-kocka jakub-kocka self-assigned this Sep 11, 2024
@jakub-kocka
Copy link
Collaborator Author

jakub-kocka commented Sep 11, 2024

FYI @leeebo

Honestly, I’m not entirely satisfied with this approach, but given the "feature freeze" (if I understood correctly) on the Windows IDF Installer, it seems like a reasonable compromise without requiring a more complex refactor.

That said, I’m open to any suggestions if you have a better approach in mind guys.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

LGTM but I would prefer to remove the code rather than adding conditions and leaving the unused code there.

By the way, do you have any benchmarks, how much time did the old approach take and how much time does the new one take? And I guess it would be good to try this from China as well.

src/InnoSetup/Environment.iss Outdated Show resolved Hide resolved
@jakub-kocka jakub-kocka requested a review from igrr October 1, 2024 07:23
Copy link

@dobairoland dobairoland left a comment

Choose a reason for hiding this comment

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

LGTM

@jakub-kocka
Copy link
Collaborator Author

@igrr, thanks for the review! I have implemented the changes as you suggested so it should be ready to merge. I have tested the installer and it works as expected.

Unfortunately, I am not able to do the merge as we agreed without your re-review and approval since you are the maintainer and requested a change.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, LGTM!

Copy link
Contributor

@leeebo leeebo left a comment

Choose a reason for hiding this comment

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

@jakub-kocka LGTM! Thanks!

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