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

Feat/windows nodes release #5049

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

wujunyi792
Copy link
Contributor

@wujunyi792 wujunyi792 commented Sep 26, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:
Added support for windows/amd64 release, including components edgecore and keadm

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This pr depends on #4968
Does this PR introduce a user-facing change?:

NONE

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 26, 2023
@kubeedge-bot kubeedge-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 26, 2023
@wujunyi792 wujunyi792 closed this Sep 26, 2023
@wujunyi792 wujunyi792 reopened this Sep 26, 2023
@wujunyi792 wujunyi792 changed the title Feat/windows nodes release [WIP]Feat/windows nodes release Sep 26, 2023
@kubeedge-bot kubeedge-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2023
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/cmd_windows.go Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/common/constant_others.go Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/reset_windows.go Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/util/nssm.go Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/util/nssm.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/common/constant_windows.go Outdated Show resolved Hide resolved
keadm/cmd/keadm/app/cmd/util/common_windows.go Outdated Show resolved Hide resolved
@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2023
@wujunyi792 wujunyi792 changed the title [WIP]Feat/windows nodes release Feat/windows nodes release Oct 26, 2023
@kubeedge-bot kubeedge-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2023
Signed-off-by: wujunyi <wu65830600@163.com>
Signed-off-by: wujunyi <wu65830600@163.com>
Signed-off-by: wujunyi <wu65830600@163.com>
@kubeedge-bot kubeedge-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2023
@ZhengXinwei-F
Copy link
Collaborator

ZhengXinwei-F commented Oct 27, 2023

This PR seems to implement linux and windows compilation in a single function.

This will make future support for more OS or windows-arm more difficult. Too many if branches will also degrade code readability.

Perhaps you can separate the logic for different operating systems into independent functions.
Like:

function build_kubeedge_release() {
    case $OS in
    linux)
    build_kubeedge_release_linux
    ;;
    windows)
    build_kubeedge_release_windows
    ;; 
    *)
    echo "not support OS $OS"
    exit 1
    ;; 
    esac 

    exit 0
}

@wujunyi792
Copy link
Contributor Author

This PR seems to implement linux and windows compilation in a single function.

This will make future support for more OS or windows-arm more difficult. Too many if branches will also degrade code readability.

Yes, it's necessary to address the problem. However, since it might be a significant task, I've opened an issue to investigate further. #5124

Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

/approve
/assign @Shelley-BaoYue

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023
@Shelley-BaoYue
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2023
@kubeedge-bot kubeedge-bot merged commit aa9f962 into kubeedge:master Oct 31, 2023
18 checks passed
@Shelley-BaoYue
Copy link
Collaborator

/milestone v1.16

@Shelley-BaoYue Shelley-BaoYue added this to the v1.16 milestone Nov 14, 2023
kubeedge-bot added a commit that referenced this pull request Nov 22, 2023
…-#5049-upstream-release-1.15

Automated cherry pick of #5049: feat: update to support release windows/amd64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants