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.

Sign up for GitHub

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

Jump to bottom

add v1 api version #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add v1 api version #81

wants to merge 1 commit into from

Conversation

lucming
Copy link

@lucming lucming commented Jul 11, 2022

@volcano-sh-bot volcano-sh-bot requested review from k82cn and wpeng102 July 11, 2022 06:48
@volcano-sh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign wpeng102
You can assign the PR to them by writing /assign @wpeng102 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@lucming lucming force-pushed the upgrade_api_version branch from 70a302d to d7d3b1e Compare July 11, 2022 06:49
@Thor-wl Thor-wl requested review from Thor-wl, hwdef and shinytang6 and removed request for k82cn July 11, 2022 07:01
@Thor-wl
Copy link
Contributor

Thor-wl commented Jul 11, 2022

@lucming Have you combine this PR with the master branch of volcano.sh/volcano and take a fully UT and e2e test locally?

Thor-wl
Thor-wl reviewed Jul 11, 2022
View reviewed changes
Copy link
Contributor

@Thor-wl Thor-wl left a comment

Choose a reason for hiding this comment

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

It generally LGTM to me. Please mind the compatibility for users who are still making use of low versions.

hwdef
hwdef reviewed Jul 11, 2022
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

By the way, don't understand the meaning of bus, should we change it to a more understandable word

pkg/apis/batch/v1/doc.go Outdated
@@ -0,0 +1,21 @@
/*
Copyright 2021 The Volcano Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright 2021 The Volcano Authors.
Copyright 2022 The Volcano Authors.

pkg/apis/batch/v1/labels.go Outdated
@@ -0,0 +1,46 @@
/*
Copyright 2021 The Volcano Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright 2021 The Volcano Authors.
Copyright 2022 The Volcano Authors.

@lucming
Copy link
Author

lucming commented Jul 12, 2022

@lucming Have you combine this PR with the master branch of volcano.sh/volcano and take a fully UT and e2e test locally?

en,volcano.sh/volcano this project changed too much,i want check more,and i will make a pr latter

@lucming
Copy link
Author

lucming commented Jul 12, 2022

It generally LGTM to me. Please mind the compatibility for users who are still making use of low versions.

okay,i will pay more attention to it,thanks.

@lucming
Copy link
Author

lucming commented Jul 12, 2022

By the way, don't understand the meaning of bus, should we change it to a more understandable word

I'm also very confused about this,can you tell me what he wants to say？

@hwdef
Copy link
Member

hwdef commented Jul 12, 2022

By the way, don't understand the meaning of bus, should we change it to a more understandable word

I'm also very confused about this,can you tell me what he wants to say？

I also don't know what this means, may need to consult other more senior maintainers

@lucming lucming force-pushed the upgrade_api_version branch 3 times, most recently from 844631e to fdd33d9 Compare July 12, 2022 09:02
@lucming
Copy link
Author

lucming commented Jul 12, 2022 •
edited
Loading

i find https://github.com/volcano-sh/apis/blob/master/pkg/apis/helpers/helpers.go this file in this project,most of them are ways to operate basic resource in k8s,it seems more reasonable to move this file to volcan.sh/volcano

@lucming lucming changed the title upgrade api version add v1 api version Jul 13, 2022
@william-wang
Copy link
Member

/hold

@william-wang
Copy link
Member

@lucming, It's great to have this pr to upgrade api version. Thank you so much.
We want to see the PR of v1 support in volcano repo prior to merging the api pr. Would you provide modification in volcano repo?

@lucming
Copy link
Author

lucming commented Jul 19, 2022

@lucming, It's great to have this pr to upgrade api version. Thank you so much. We want to see the PR of v1 support in volcano repo prior to merging the api pr. Would you provide modification in volcano repo?

I have written a version of the code to upgrade the volcano api version, but there are other things were delayed so have not mentioned pr.

@lucming
upgrade api version
062ae57 
Signed-off-by: lucming <[email protected]>
@lucming lucming force-pushed the upgrade_api_version branch from fdd33d9 to 062ae57 Compare July 26, 2022 10:30
@lucming lucming mentioned this pull request Jul 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thor-wl Thor-wl Thor-wl left review comments

@hwdef hwdef hwdef left review comments

@wpeng102 wpeng102 Awaiting requested review from wpeng102

@shinytang6 shinytang6 Awaiting requested review from shinytang6

Assignees
No one assigned
Labels
do-not-merge/hold size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lucming @volcano-sh-bot @Thor-wl @hwdef @william-wang