Skip to content

Conversation

@menglingwei
Copy link
Contributor

alibabacloud machine-api

xiaobing.meng added 2 commits August 2, 2019 11:15
@openshift-ci openshift-ci bot added do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. and removed do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Jul 28, 2021
@elmiko
Copy link
Contributor

elmiko commented Jul 28, 2021

you can drop the OWNERS file commit from this, i have opened #2 to add it.

@menglingwei
Copy link
Contributor Author

I removed the OWNERS files

@elmiko
Copy link
Contributor

elmiko commented Jul 29, 2021

thanks @menglingwei , i think we should probably merge this as is and then start running the ci against it to see if there are any changes needed.
/lgtm
cc @JoelSpeed

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@JoelSpeed
Copy link
Contributor

Since we seem to just be copying the alibaba version of this repo, is there any reason not to have forked the repo rather than doing this PR? If we were to fork, we would retain the history right?

@elmiko
Copy link
Contributor

elmiko commented Aug 2, 2021

Since we seem to just be copying the alibaba version of this repo, is there any reason not to have forked the repo rather than doing this PR? If we were to fork, we would retain the history right?

good question, i had wondered about that but in discussion about setup with @menglingwei this was the methodology we discussed (create new branch, then create a pr).

i believe you are correct about the history aspect of the fork.

@menglingwei
Copy link
Contributor Author

@elmiko @JoelSpeed You're referring to the repo fork from Alibaba?

@elmiko
Copy link
Contributor

elmiko commented Aug 2, 2021

@menglingwei correct. Joel is saying that we could have made the request to create this repository use a fork from the https://github.com/AliyunContainerService/cluster-api-provider-alibabacloud repository. then we would keep the history and the repo would be populated from there.

if that sounds better, i can investigate if we can do that.

@menglingwei
Copy link
Contributor Author

@menglingwei correct. Joel is saying that we could have made the request to create this repository use a fork from the https://github.com/AliyunContainerService/cluster-api-provider-alibabacloud repository. then we would keep the history and the repo would be populated from there.

if that sounds better, i can investigate if we can do that.

@elmiko I think it's doable. You can fork from https://github.com/AliyunContainerService/cluster-api-provider-alibabacloud .
If there is any problem, you can assign issues to me

@menglingwei
Copy link
Contributor Author

@elmiko But in our(alibaba) repo, the master branch is too old. so you can use branch machine-api-v0.4.1.
Next, we'll merge the branch ”machine-api-v0.4.1" into the master branch.

@elmiko
Copy link
Contributor

elmiko commented Aug 2, 2021

thanks @menglingwei , i will do some investigation to see how we can set this up. we might need to delete and remake this repo, but i will update you before we do anything.

@elmiko
Copy link
Contributor

elmiko commented Aug 2, 2021

@menglingwei , @JoelSpeed and i are discussing the options for bringing the code over here and we have a couple questions. as we see it, there are 3 options we could do:

  1. merge this pr
  2. recreate this repo as a fork from the machine-api-v0.4.1 branch of the aliyuncontainerservice repo
  3. push the machine-api-v0.4.1 branch from the aliyuncontainerservice repo into this repo

2 and 3 allow us to keep the history of that branch (39 commits).

this had led us to ask a couple questions:

  1. do you plan to continue working on the aliyuncontainerservice repo? (ie is there value in making this a fork of that repo)
  2. assuming yes to 1, do you plan to promote patches from that repo to this one?

if the answer is no, then i think we can just merge this patch and keep working here. but if you would like to preserve some sort of fork between the two, then we should make sure that the histories are the same.

i don't have a personal preference. the history on the aliyuncontainerservice repo is not that long, so if the plan is to continue work here then i think we should just merge this. but i want to make sure we explore the option of preserving the history, if needed.

@menglingwei
Copy link
Contributor Author

@menglingwei , @JoelSpeed and i are discussing the options for bringing the code over here and we have a couple questions. as we see it, there are 3 options we could do:

  1. merge this pr
  2. recreate this repo as a fork from the machine-api-v0.4.1 branch of the aliyuncontainerservice repo
  3. push the machine-api-v0.4.1 branch from the aliyuncontainerservice repo into this repo

2 and 3 allow us to keep the history of that branch (39 commits).

this had led us to ask a couple questions:

  1. do you plan to continue working on the aliyuncontainerservice repo? (ie is there value in making this a fork of that repo)
  2. assuming yes to 1, do you plan to promote patches from that repo to this one?

if the answer is no, then i think we can just merge this patch and keep working here. but if you would like to preserve some sort of fork between the two, then we should make sure that the histories are the same.

i don't have a personal preference. the history on the aliyuncontainerservice repo is not that long, so if the plan is to continue work here then i think we should just merge this. but i want to make sure we explore the option of preserving the history, if needed.

@elmiko We will always update and maintain aliyunContainerService repo. So you can fork from this repo.

@elmiko
Copy link
Contributor

elmiko commented Aug 3, 2021

thanks @menglingwei , i will discuss with Joel and get this repo configured properly.

@elmiko
Copy link
Contributor

elmiko commented Aug 3, 2021

talked with @JoelSpeed, he is going to bring the branch over and then propose PR to merge it. this way we can preserve the history from the AliyunContainerService repo.

@menglingwei
Copy link
Contributor Author

@elmiko @JoelSpeed Thank you guys. Let's move on to the next step.

@elmiko
Copy link
Contributor

elmiko commented Aug 9, 2021

on the topic of CI, i have this PR setup to enable the basic unit tests on this repo when we are ready. openshift/release#20668

@menglingwei
Copy link
Contributor Author

I don't think we have any CI set up right now, could you show me where we are seeing failures please?

it's not failures. I found in my rep submitted on https://github.com/menglingwei/cluster-api-provider-alibaba/tree/machine-api-v0.4.0 and your submission (https://github.com/openshift/cluster-api-provider-alibaba/tree/machine-api-v0.4.1-no-bin) is different. Therefore, I think my handling should be something not quite right. But I don't know where it happened

@JoelSpeed
Copy link
Contributor

Do you know what commands you ran to create your branch/to reset it to match mine?

@menglingwei
Copy link
Contributor Author

Do you know what commands you ran to create your branch/to reset it to match mine?

  1. git remote add openshift https://github.com/openshift/cluster-api-provider-alibaba
  2. git fetch openshift
  3. git reset --hard openshift/machine-api-v0.4.1-no-bin

And then ,I resolved the code conflict. and commited it

@JoelSpeed
Copy link
Contributor

That sounds correct as far as I can tell. I'm sorry I'm not understanding what the issue exactly is, would you be able to spell out exactly what differences/problems you're seeing?

@menglingwei
Copy link
Contributor Author

That sounds correct as far as I can tell. I'm sorry I'm not understanding what the issue exactly is, would you be able to spell out exactly what differences/problems you're seeing?

This is my commit history https://github.com/menglingwei/cluster-api-provider-alibaba/commits/machine-api-v0.4.0

This is yours https://github.com/openshift/cluster-api-provider-alibaba/commits/machine-api-v0.4.1-no-bin

I found commit history havs a little different. So I think maybe I'm not doing it right.

@JoelSpeed
Copy link
Contributor

Got it! Ok, I see the differences, I think your branch is ok though, no issue regarding the reset :)

I would suggest dropping this https://github.com/menglingwei/cluster-api-provider-alibaba/commit/1697c637e27caa6189734da29fb2e51fb4b4cf50, otherwise I think history wise we are all good and things should be resolved regarding the binaries

@menglingwei
Copy link
Contributor Author

Ok. Thank you.I'm going to try to drop the commit menglingwei@1697c63

@kwoodson
Copy link
Contributor

@menglingwei Were you able to drop the commit? Please let us know if we can assist. Thanks!

@kwoodson
Copy link
Contributor

kwoodson commented Aug 11, 2021

@menglingwei @JoelSpeed I attempted working with the no-bin branch and ran into a few issues.

I attempted to make generate and saw that the generated files need to be updated:

make generate
podman run --rm -e CGO_ENABLED=0 -e GOARCH=amd64 -e GOOS=linux -v "/home/kwoodson/go/src/github.com/openshift/cluster-api-provider-alibaba":/go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud:Z -w /go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud openshift/origin-release:golang-1.15 go generate ./pkg/... ./cmd/...
podman run --rm -e CGO_ENABLED=0 -e GOARCH=amd64 -e GOOS=linux -v "/home/kwoodson/go/src/github.com/openshift/cluster-api-provider-alibaba":/go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud:Z -w /go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud openshift/origin-release:golang-1.15 hack/goimports.sh .
hack/verify-diff.sh
diff --git a/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go b/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
index b3a352a2..38d9670b 100644
--- a/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
+++ b/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
@@ -21,7 +21,7 @@ limitations under the License.
 package v1beta1
 
 import (
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
make: *** [Makefile:135: goimports] Error 1

@menglingwei Can you also verify that the make build and other scripts produce the binaries? I was able to build with podman build . .

@menglingwei
Copy link
Contributor Author

@menglingwei @JoelSpeed I attempted working with the no-bin branch and ran into a few issues.

I attempted to make generate and saw that the generated files need to be updated:

make generate
podman run --rm -e CGO_ENABLED=0 -e GOARCH=amd64 -e GOOS=linux -v "/home/kwoodson/go/src/github.com/openshift/cluster-api-provider-alibaba":/go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud:Z -w /go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud openshift/origin-release:golang-1.15 go generate ./pkg/... ./cmd/...
podman run --rm -e CGO_ENABLED=0 -e GOARCH=amd64 -e GOOS=linux -v "/home/kwoodson/go/src/github.com/openshift/cluster-api-provider-alibaba":/go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud:Z -w /go/src/github.com/AliyunContainerService/cluster-api-provider-alibabacloud openshift/origin-release:golang-1.15 hack/goimports.sh .
hack/verify-diff.sh
diff --git a/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go b/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
index b3a352a2..38d9670b 100644
--- a/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
+++ b/pkg/apis/alibabacloudprovider/v1beta1/zz_generated.deepcopy.go
@@ -21,7 +21,7 @@ limitations under the License.
 package v1beta1
 
 import (
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
make: *** [Makefile:135: goimports] Error 1

@menglingwei Can you also verify that the make build and other scripts produce the binaries? I was able to build with podman build . .

Ok. I have a try

@menglingwei
Copy link
Contributor Author

@menglingwei Were you able to drop the commit? Please let us know if we can assist. Thanks!

I'm working on it.If I can't solve it, I'll ask for your help. Thank you very much.

@menglingwei
Copy link
Contributor Author

@JoelSpeed I reset my branch,and force push it. And then i found the PR is closed. Is there something wrong with my operation?

@menglingwei
Copy link
Contributor Author

make generate

Sorry. I don't have a podman on my dev env.so i need to run it first.

@JoelSpeed
Copy link
Contributor

The problem seems to be that the branch now doesn't share a common ancestor with our main branch. I want to make sure the two repositories are matched going forward so perhaps we need to reset our repo to match yours.

If you are confident that the branch is correct, I'd suggest you merge your branch into your main/master, once it's in your main, I can reset our repository to match yours, then we can make the openshift specific changes on top of that.

Let me check with @kwoodson and @elmiko on our side

@menglingwei
Copy link
Contributor Author

The problem seems to be that the branch now doesn't share a common ancestor with our main branch. I want to make sure the two repositories are matched going forward so perhaps we need to reset our repo to match yours.

If you are confident that the branch is correct, I'd suggest you merge your branch into your main/master, once it's in your main, I can reset our repository to match yours, then we can make the openshift specific changes on top of that.

Let me check with @kwoodson and @elmiko on our side

�Yes. I already check my branch. and run make build in my local env.

@menglingwei
Copy link
Contributor Author

The problem seems to be that the branch now doesn't share a common ancestor with our main branch. I want to make sure the two repositories are matched going forward so perhaps we need to reset our repo to match yours.
If you are confident that the branch is correct, I'd suggest you merge your branch into your main/master, once it's in your main, I can reset our repository to match yours, then we can make the openshift specific changes on top of that.
Let me check with @kwoodson and @elmiko on our side

�Yes. I already check my branch. and run make build in my local env.

I already merge into my main/master

@JoelSpeed
Copy link
Contributor

Oh cool, you're way ahead of me then 😅 Let me talk to Mike and Kenny later and we will work out exactly what we want to do, my gut says to now reset our master branch to match yours, then this PR should just be the openshift specific parts

@menglingwei
Copy link
Contributor Author

perfect.

@menglingwei
Copy link
Contributor Author

@JoelSpeed I see you've created a new branch.

@menglingwei
Copy link
Contributor Author

@JoelSpeed @elmiko What are we going to do next? What do you need me to do?

@elmiko
Copy link
Contributor

elmiko commented Aug 16, 2021

@menglingwei i think Kenny, Joel and myself need to sync up about the next steps. (apologies for the delay, we had a holiday on friday)

@JoelSpeed
Copy link
Contributor

So I was looking at the branch that we have today in the master of the Aliyun fork, it still has binaries in it because it was merged with the old verion.

What I've done is remove the old version in https://github.com/openshift/cluster-api-provider-alibaba/commits/sync-baba-ocp?before=6e39b84ab92b663a4bfa98e7fd1b68199a048faf+35&branch=sync-baba-ocp

So this branch is the one I'd prefer to merge to our master (it's all of your work on the current version, minus any binaries or history from the previous version). To make that work, and to be in sync, you will need to reset your master branch to b0a0adf. Once that's done, we should have the same history apart from the final two commits we have added on top. This will allow us to automate rebasing in the future and pull in your changes.

Let me know if you have concerns about this or need further information to understand what I'm trying to achieve here :)

@menglingwei
Copy link
Contributor Author

Have you finished the above work now? So I just need to reset our branch to b0a0adf

@JoelSpeed
Copy link
Contributor

I think so yes, if you're happy to reset it to b0a0adf then I think we can get our fork synced from there

@menglingwei
Copy link
Contributor Author

I think so yes, if you're happy to reset it to b0a0adf then I think we can get our fork synced from there

ok. let me reset to b0a0adf . when i done it ,i will let you know.

@menglingwei
Copy link
Contributor Author

@JoelSpeed Now I'm looking at how to clean up the history files in .git and make package smaller.

@menglingwei
Copy link
Contributor Author

I have updated repo https://github.com/AliyunContainerService/cluster-api-provider-alibabacloud

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