Add description to resource capi created by jichenjc · Pull Request #870 · kubernetes-sigs/cluster-api-provider-openstack

jichenjc · 2021-05-19T04:17:39Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #868

please note server description not added yet
gophercloud/gophercloud#1505

I need drive that...

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

k8s-ci-robot · 2021-05-19T04:17:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jichenjc]

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

hidekazuna · 2021-05-19T09:08:44Z

Adding clusterName to function argument only for description is not good. Can we add clusterName to each Service struct and use it?

jichenjc · 2021-05-19T09:16:46Z

Adding clusterName to function argument only for description is not good. Can we add clusterName to each Service struct and use it?

I think we did this all the time :) e.g do some logging etc to add a param, I am not sure why it's not good
but I agree this might be a good suggestion to include this into Service object

sbueringer · 2021-05-19T15:16:11Z

Adding clusterName to function argument only for description is not good. Can we add clusterName to each Service struct and use it?

I think we did this all the time :) e.g do some logging etc to add a param, I am not sure why it's not good
but I agree this might be a good suggestion to include this into Service object

I think I would prefer your current implementation righ tnow. If we put it into the service we have to make all those funcs methods.

jichenjc · 2021-05-20T00:00:07Z

ok, then please check whether current code is ok ..thanks

sbueringer · 2021-05-20T03:20:59Z

ok, then please check whether current code is ok ..thanks

Yup will do. Probably will need until next week. Pretty busy right now.

hidekazuna · 2021-05-20T05:33:49Z

Adding clusterName to function argument only for description is not good. Can we add clusterName to each Service struct and use it?

I think we did this all the time :) e.g do some logging etc to add a param, I am not sure why it's not good
but I agree this might be a good suggestion to include this into Service object

I think I would prefer your current implementation righ tnow. If we put it into the service we have to make all those funcs methods.

Almost(All?) done by #871, PTAL. @jichenjc It is ok not to add clusterName into each Service struct in this PR 😸

hidekazuna · 2021-05-21T08:01:51Z

pkg/cloud/services/compute/instance.go

-		PortID: portID,
+		Name:        trunkName,
+		PortID:      portID,
+		Description: fmt.Sprintf("Created by cluster-api-provider-openstack cluster %s", clusterName),


This description value is common among packages. Can we create common func among packages?

agree, I can modify this ~

hidekazuna · 2021-05-24T06:29:46Z

/lgtm

sbueringer

otherwise lgtm

sbueringer · 2021-05-24T13:04:29Z

pkg/utils/utils.go

+limitations under the License.
+*/
+
+package utils


having a generic utils package named utils (without further sub packages) is an absolute anti-pattern in go. Please create a sub-package with a less-generic name.

agree with @sbueringer 😄

hidekazuna · 2021-05-24T23:13:05Z

/lgtm cancel

pkg/cloud/services/compute/instance.go

hidekazuna · 2021-05-25T00:30:35Z

pkg/cloud/services/compute/instance.go

 }

-func (s *Service) getOrCreateTrunk(eventObject runtime.Object, trunkName, portID string) (*trunks.Trunk, error) {
+func getOrCreateTrunk(eventObject runtime.Object, clusterName, trunkName, portID string) (*trunks.Trunk, error) {


This seems wrongly rebased. Now we need (s *Service)

hidekazuna · 2021-05-25T00:30:59Z

pkg/cloud/services/networking/network.go

 	if len(subnetList) == 0 {
 		var err error
-		subnet, err = s.createSubnet(openStackCluster, subnetName)
+		subnet, err = createSubnet(openStackCluster, clusterName, subnetName)


This seems wrongly rebased. Now we need s.

hidekazuna · 2021-05-25T00:31:15Z

pkg/cloud/services/networking/network.go

 }

-func (s *Service) createSubnet(openStackCluster *infrav1.OpenStackCluster, name string) (*subnets.Subnet, error) {
+func createSubnet(openStackCluster *infrav1.OpenStackCluster, clusterName string, name string) (*subnets.Subnet, error) {


This seems wrongly rebased. Now we need (s *Service)

hidekazuna · 2021-05-25T00:31:33Z

pkg/cloud/services/networking/router.go

 	if len(routerList) == 0 {
 		var err error
-		router, err = s.createRouter(openStackCluster, routerName)
+		router, err = createRouter(openStackCluster, clusterName, routerName)


This seems wrongly rebased. Now we need s.

sbueringer · 2021-05-25T05:15:06Z

/lgtm

sbueringer · 2021-05-25T05:17:49Z

@jichenjc imho you can squash & merge

sbueringer · 2021-05-25T07:51:30Z

@jichenjc I actually meant squash the commit and then merge via Prow with /hold cancel (not via the GitHub button which only admins have access to) :)

jichenjc · 2021-05-25T07:52:21Z

@sbueringer @hidekazuna I found I can squahs and merge this directly which is dangerous .. I think better leave this to a flow such as /hold cancel , /lgtm etc, not sure we can do that ?

If you think I need revert this , please let me know

sbueringer · 2021-05-25T07:53:53Z

See: #870 (comment)

I think we don't have to revert it. It just added your PR as one commit on the master branch.
The only impact it has is that the release note tool won't pick it up, but that's not a problem.

We just should keep doing it via the regular process the next time.

* Add description to resource capi created

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 19, 2021

k8s-ci-robot requested review from prankul88 and sbueringer May 19, 2021 04:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 19, 2021

hidekazuna reviewed May 22, 2021

View reviewed changes

jichenjc force-pushed the add_description-1 branch 2 times, most recently from d7661df to 3fbd638 Compare May 24, 2021 01:34

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 24, 2021

k8s-ci-robot assigned hidekazuna May 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2021

sbueringer reviewed May 24, 2021

View reviewed changes

sbueringer mentioned this pull request May 24, 2021

🏃 Refactor: use Service struct field more #871

Merged

3 tasks

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 24, 2021

Add description to resource capi created

76b25dd

jichenjc force-pushed the add_description-1 branch from b28ad43 to f9910e4 Compare May 25, 2021 00:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

hidekazuna suggested changes May 25, 2021

View reviewed changes

package utils => names

f3088d8

jichenjc force-pushed the add_description-1 branch from f9910e4 to f3088d8 Compare May 25, 2021 00:40

k8s-ci-robot assigned sbueringer May 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

sbueringer mentioned this pull request May 25, 2021

✨ Add feature to create ports with custom options #876

Merged

7 tasks

jichenjc merged commit 55ec7fa into kubernetes-sigs:master May 25, 2021

tobiasgiese mentioned this pull request Jul 8, 2021

Cleanup loadbalancers created by ccm #842

Closed

EmilienM mentioned this pull request Jan 31, 2023

Manual rebase on upstream openshift/cluster-api-provider-openstack#252

Closed

pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024

Add description to resource capi created (kubernetes-sigs#870)

a1de15f

* Add description to resource capi created

Conversation

jichenjc commented May 19, 2021

Uh oh!

k8s-ci-robot commented May 19, 2021

Uh oh!

hidekazuna commented May 19, 2021

Uh oh!

jichenjc commented May 19, 2021

Uh oh!

sbueringer commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jichenjc commented May 20, 2021

Uh oh!

sbueringer commented May 20, 2021

Uh oh!

hidekazuna commented May 20, 2021

Uh oh!

hidekazuna May 21, 2021

Choose a reason for hiding this comment

Uh oh!

jichenjc May 24, 2021

Choose a reason for hiding this comment

Uh oh!

hidekazuna commented May 24, 2021

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

sbueringer May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidekazuna May 24, 2021

Choose a reason for hiding this comment

Uh oh!

hidekazuna commented May 24, 2021

Uh oh!

Uh oh!

hidekazuna May 25, 2021

Choose a reason for hiding this comment

Uh oh!

hidekazuna May 25, 2021

Choose a reason for hiding this comment

Uh oh!

hidekazuna May 25, 2021

Choose a reason for hiding this comment

Uh oh!

hidekazuna May 25, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer commented May 25, 2021

Uh oh!

sbueringer commented May 25, 2021

Uh oh!

sbueringer commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jichenjc commented May 25, 2021

Uh oh!

sbueringer commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbueringer commented May 19, 2021 •

edited

Loading

sbueringer May 24, 2021 •

edited

Loading

sbueringer commented May 25, 2021 •

edited

Loading