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

[kubeapps-apis] Apply renaming at the code #2971

Merged
merged 13 commits into from
Jun 11, 2021
Merged

[kubeapps-apis] Apply renaming at the code #2971

merged 13 commits into from
Jun 11, 2021

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Jun 10, 2021 •
edited

Description of the change

This is a draft PR with a bunch of changes required after #2965. The motivation is to avoid other being blocked because of the breaking changes. So, if you are a plug-in developer, please feel free to add some of these changes in your local.
I've also modified a couple of tests, they are just minor changes introducing the corev1.Context{} object to avoid nil references.

Tomorrow I will continue updating so the test suite passes.

Benefits

Code will work again in the main branch

Possible drawbacks

I haven't thoroughly tested all the methods, but it's worthwhile just to land something working and send a followup PR with further changes if needed.

Applicable issues

Additional information

Note that, due to the renaming, if testing the API using the non gRPC endpoint, the query params ?namespace=xx and ?cluster=yy became renamed to ?context.namespace=xx and ?context.cluster=yy

@antgamdia
Update proto files with the new names and fields
f7ee34b
@antgamdia
Update generated api docs
51a0e28
@antgamdia
Add generated files (will break plugins)
b6dabc3
@antgamdia
Fix minor typo
ab79ed2
@antgamdia
Changes after the renaming
55c78d6
@antgamdia
Merge branch 'master'
bd81688 
Conflicts:
	cmd/kubeapps-apis/docs/kubeapps-apis.swagger.json
	cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.go
	cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.gw.go
	cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages_grpc.pb.go
	cmd/kubeapps-apis/gen/plugins/fluxv2/packages/v1alpha1/fluxv2.pb.go
	cmd/kubeapps-apis/gen/plugins/fluxv2/packages/v1alpha1/fluxv2.pb.gw.go
	cmd/kubeapps-apis/gen/plugins/fluxv2/packages/v1alpha1/fluxv2_grpc.pb.go
	cmd/kubeapps-apis/gen/plugins/kapp_controller/packages/v1alpha1/kapp_controller.pb.go
	cmd/kubeapps-apis/gen/plugins/kapp_controller/packages/v1alpha1/kapp_controller.pb.gw.go
	cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/packages.proto
	cmd/kubeapps-apis/proto/kubeappsapis/plugins/fluxv2/packages/v1alpha1/fluxv2.proto
@antgamdia
Changes after the latest msg edits
14c6529
@antgamdia
Comment tests while working on making them work
11ba2bc
@antgamdia
Merge branch 'master' into 2945-renameMessages-plugins
7b6a862
@antgamdia antgamdia mentioned this pull request Jun 10, 2021
Closed
absoludity
absoludity approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Great, thanks for jumping on to this straight away. I'm leaving a +1 as even in its current state (with tests commented out) it'd be better than broken master, but great if you get the tests updated too first.

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go Outdated
@@ -23,7 +23,7 @@ import (
"k8s.io/client-go/dynamic"

corev1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/fluxv2/packages/v1alpha1"
v1alpha1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/fluxv2/packages/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should only need an explicit name for the imported module if you're using something other than the default name of "v1alpha1" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, I don't know either, I think I just auto-imported with vs code, but, yep, you're right, the explicit name is no required at all. Thank you!

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server.go Outdated
@@ -33,7 +33,7 @@ import (
log "k8s.io/klog/v2"

corev1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/kapp_controller/packages/v1alpha1"
v1alpha1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/kapp_controller/packages/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, unsure why you're adding an explicit name for the import when it matches the default?

cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/server_test.go Outdated
@@ -21,6 +21,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
corev1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
v1alpha1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/kapp_controller/packages/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@antgamdia antgamdia marked this pull request as ready for review June 11, 2021 09:05
@gfichtenholt
Copy link
Contributor

Mmmm, I can't say I share @absoludity sentiment on this. I just discovered this PR and am a bit horrified at the upcoming merge I will need to perform in fluxv2 plugin server code, where I have been making changes for several days now. I am not asking you not to go ahead with this PR, but I AM saying it would have been nice to notify me ahead of time, as I own fluxv2 plug in and you're making major changes there. Would have saved me cycles not to make changes to the same file. Plus, I think its a bit premature to be making these changes at this point. The design spec is far from reviewed let alone approved.

@antgamdia
Copy link
Contributor Author

but I AM saying it would have been nice to notify me ahead of time, as I own fluxv2 plug in and you're making major changes there. Would have saved me cycles not to make changes to the same file.

I totally understand your concerns, it's true we have performed a lot of changes here (especially in #2945), but the vast majority are almost automatic replacements. That said, I don't mind when you send the PR, I can just jump in and edit your PR adding these changes back again.

However, let me know if I'm wrong, but all the information about the current status of the tasks has been updated in real-time in our source of truth board: https://github.com/kubeapps/kubeapps/projects/11

Plus, I think its a bit premature to be making these changes at this point. The design spec is far from reviewed let alone approved.

Our approach here has been just trying to perform laser-focused changes just in AvailablePackage and InstalledPackage, so we minimize the number of changes in each step. What would you suggest here?

@absoludity
Copy link
Contributor

Mmmm, I can't say I share @absoludity sentiment on this. I just discovered this PR and am a bit horrified at the upcoming merge I will need to perform in fluxv2 plugin server code, where I have been making changes for several days now.

Argh... sorry @gfichtenholt, I thought you and I did talk about this during our call (with Dimitri) earlier when I mentioned that @antgamdia would be updating the naming in the core.packages.v1alpha1 which would affect the plugins. I was perhaps not clear enough, sorry. Do you remember the conversation though? (you said something along the lines of not minding as long as you're not blocking anyone, from memory, but sounds like I hadn't communicated clearly that it would involve).

If you want to ignore the change and just continue on your branch, then push it as a draft PR, I'm happy to resolve the conflicts.

I am not asking you not to go ahead with this PR, but I AM saying it would have been nice to notify me ahead of time, as I own fluxv2 plug in and you're making major changes there.

I thought I did :/

Would have saved me cycles not to make changes to the same file. Plus, I think its a bit premature to be making these changes at this point. The design spec is far from reviewed let alone approved.

The design spec as a whole, yes, but the core.packages.v1alpha1 messages are something I have been working hard to whittle down to something we agree on, for this reason - this is #2944 that I'd assigned to myself for this iteration and estimated 3-4 days. The task Antonio is doing is #2945 which I initiated as a card (Antonio converted that to an issue) and I outlined in the first comment there how I'd suggest progressing (which is exactly what Antonio has done).

Both of these are pre-requisites to #2946 which is where I assume you'd be continuing working on the flux plugin.

Not an excuse for lack of clear communication, but another idea too: It might be worth subscribing to comments on all issues so you can voice your thoughts and contribute while we're all planning these too.

@antgamdia antgamdia merged commit 9cedd27 into vmware-tanzu:master Jun 11, 2021
@antgamdia antgamdia deleted the 2945-renameMessages-plugins branch June 11, 2021 10:15
@gfichtenholt
Copy link
Contributor

gfichtenholt commented Jun 11, 2021 •
edited

@absoludity yeah, wasn't clear to me the extent of the changes. Possibly my bad.
@antgamdia at this point the plugin code doesn't compile so this change had to go in to fix it. That's why I said go ahead.
Now, someone has to bite the proverbial bullet and do the merge. I guess it'll be me, I have the most context. I will do over the weekend. Just hope the communication gets handled better next time around
That said I still don't understand the rush. The spec has not been reviewed, a lot more changes are likely coming. Wouldn't it make more sense to wait until the spec is approved and then make the changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absoludity absoludity absoludity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@antgamdia @gfichtenholt @absoludity