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

upgrade operator-sdk to v0.9.0 #88

Open
wants to merge 22 commits into
base: master
from

Conversation

@jsanda
Copy link
Contributor

commented Aug 5, 2019

This PR is for #75.

I want to make a note of a breaking change. metrics.CreateMetricsService(ctx, metricsPort) was removed and replaced with metrics.CreateMetricsService(ctx, cfg, servicePorts). See operator-framework/operator-sdk#1560 for details.

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@allamand CircleCI is still using v0.7.0 of operator-sdk, but I updated Gopkg.toml. Do you know what the issue is? Looks like we might need make force-get-deps.

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I think I need to rebuild image for circleCI with new version

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I think I need to rebuild image for circleCI with new version

Is that something that I can do?

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I did the following before committing/pushing,

$ make clean
$ make get-deps
$ make build
$ make unit-test

Unit tests passed. @allamand are you suggesting I do something else?

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I think to reproduce on local what is doing on circleCI you must use

Make docker-build

But first you need to build new image with new operator dsl version with

Make build-ci-image

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@allamand I think I finally got it sorted out. Here is a summary of what I did to get past the initial errors based on your instructions:

  • Updated Makefile to use a different build image so that I could push/pull
    • This allowed to build with operator-sdk v0.9.0 executable
  • Updated CircleCI config to call make force-get-deps
    • vendor directory still had operator-sdk v0.9.0 which resulted in compilation error
  • Added make clean to CircleCI config because the build was complaining about missing file in vendor directory

I got past the initial errors after these changes, and unit tests are passing.

I know that some of my changes, particularly those to the CircleCI config, may be temporary; however, would similar steps have to be performed when other dependences change?

Can we please resume the CI build so that e2e tests are run?

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@jsanda in my first mind, I was thinking to use of go module with new version of sdk to avoid problem with vendor directory.

Do you think it's better doing this in this PR or in another ?

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@jsanda there is an error while executing test:

vendor/github.com/operator-framework/operator-sdk/internal/pkg/scaffold/crd.go:94:4: unknown field 'Repo' in struct literal of type "github.com/Orange-OpenSource/cassandra-k8s-operator/vendor/sigs.k8s.io/controller-tools/pkg/crd/generator".Generator

Did you manage execute them locally (using make docker-e2e?

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@allamand I am fine with using the go module. Does that mean we introduce support for go modules in this PR? I will need to check to see how to do this. Not sure if it is as simple as something like go build -mod vendor.

With respect to the error, I did not run the make docker-e2e. I will though.

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

not sure of the complexity to introduce go module if it's not too much we can try to include here, othewise will do it in another

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

@allamand I spent a couple hours trying to get things working with module. I did not have much success. I kept running into different compilation errors.

To be honest, I don't have a strong preference as to whether or not we try to introduce modules as part of this PR or as part of another. I would like to see the move to modules and I would like to see operator-sdk upgraded :) I am happy to help with the migration to modules but may need some help. Maybe we can sync up on slack?

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

FYI, I opened an issue on the operator-sdk project asking for some help with migrating to modules since I haven't found any docs.

@cscetbon

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@jsanda did you try starting it from scratch and see ?

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@cscetbon I did go through upgrading the example application in the operator-sdk docs. Initially I ran into the same problem. It turned out that I was missing a step. Running go mod tidy was needed. When I get a chance I need to update and close my operator-sdk ticket.

I still had problems here though even after running go mod tidy. I will diff our go.mod with the one from the example application and hopefully that will make it easier to figure out what dependences need to be fixed.

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I just pushed a commit that finally gets operator-sdk generate k8s working with go.mod.

Running operator-sdk build as is done in the build target still needs some work. I will try to address that next.

The Dockerfile generated by v0.9.0 of operator-sdk is a good bit different from what we have. I may need some help with updating it.

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I made more progress with my last commit df5d80c. The error in CircleCI looks specific to that environment. I get a different when I run make unit-test locally.

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

With commit a209dfe I get the same test failure in CircleCI that I get locally.

I also still need to revert the image used in the CircleCI config. I changed it to use on from my GCR repo to get past some errors previously.

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@allamand I got unit tests passing. I have had the CirlcCI configured to use the image gcr.io/cassandra-k8s/casskop-build:v0.9.0. I need a orangeopensource/casskop-build:v0.9.0 image to proceed, or run the e2e tests with the current image and see if we have everything passing. Then once all tests pass, create the v0.9.0 image.

)

replace (
k8s.io/api => k8s.io/api v0.0.0-20190222213804-5cb15d344471

This comment has been minimized.

Copy link
@cscetbon

cscetbon Aug 16, 2019

Contributor

why is that one needed when you could just change it in the requires ?

This comment has been minimized.

Copy link
@jsanda

jsanda Aug 17, 2019

Author Contributor

I tried doing that, but the replace directive keeps getting added back. To be honest, I am not sure why as of yet. I have more focused on just getting things working and have not taken much time to investigate.

This comment has been minimized.

Copy link
@cscetbon

cscetbon Aug 22, 2019

Contributor

@jsanda when everything is ready let's try to take a new stab at it ;)

@@ -216,9 +226,12 @@ func helperCreateCassandraCluster(t *testing.T, cassandraClusterFileName string)
assert.Equal(cc.Status.Phase, api.ClusterPhaseRunning)

for _, dcRackName := range dcRackNames {

This comment has been minimized.

Copy link
@cscetbon

cscetbon Aug 16, 2019

Contributor

You don't really need a variable, just delete dcRackNames and use cc.GetDCRackNames() here.

APIVersion: "v1",
},
}
//Raw: &metav1.ListOptions{

This comment has been minimized.

Copy link
@cscetbon

cscetbon Aug 16, 2019

Contributor

Any reason why we don't need that code anymore ? And if it's not needed, let's remove it definitely

This comment has been minimized.

Copy link
@jsanda

jsanda Aug 17, 2019

Author Contributor

Any reason why we don't need that code anymore ?

It was fixed in kubernetes-sigs/controller-runtime#213. I am removing the commented out code.

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@jsanda I just trigger tests to see how they doing

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

I just pushed a couple commits to address comments and to fix the e2e tests. I changed WORKDIR in Makefile to be outside of $GOPATH/src so that modules are automatically enabled. This is consistent with what I have done in other places.

@allamand can you restart tests again please?

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@allamand Can you run the e2e tests? I did some testing locally to make sure the docker build worked and one of the e2e tests. I need to test CircleCI now.

@cscetbon

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

@jsanda I just did

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

@cscetbon Thanks.

I just pushed another change to set GO111MODULE for docker targets. Can we start e2e tests again please?

@cscetbon

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@jsanda /tests

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@cscetbon thanks for running tests. Can I kick them off now as well?

@cscetbon and/or @allamand now that all tests pass, we need a orangeopensource/casskop-build:v0.9.0 image and then I change the CircleCI config in my branch back to use the image.

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@Orange-cscetbon

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@allamand Can you have a look at it ?
@jsanda can you resolve the current conflicts on the Makefile ?

jsanda added some commits Aug 5, 2019

Need to run docker-get-deps before docker-build
I am not sure if docker-get-deps should be a hard dependency for docker-build
but definitely need to update deps right now prior to docker-build since we
are upgrading a dependency.
create a working go.mod file
With this commit I can finally get `operator-sdk generate k8s` to run
successfully.

I also removed dep references from Makefile.

Building the docker image still needs some work.
revert WORKDIR
The docker build was failing for me locally with the previous value.

@jsanda jsanda force-pushed the jsanda:upgrade-operator-sdk branch from 1ed07fb to de1550c Aug 22, 2019

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

t.Fatalf("reconcile: (%v)", err)
//We recall Reconcile to update Next rack
res, err = rcc.Reconcile(req)
if err != nil {

This comment has been minimized.

Copy link
@cscetbon

cscetbon Aug 22, 2019

Contributor

You can do it in one line with if _, err = rcc.Reconcile(req); err != nil {
I also removed res as it's not used

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@cscetbon can you start the e2e tests again please?

@jsanda

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Thanks!

Now we're just waiting on an orangeopensource/casskop-build:v0.9.0 image. When can we expect that?

@allamand

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.