refactor Name() and ExtendedName(c) methods #865

olasaadi99 · 2024-09-10T09:06:34Z

No description provided.

adisos

added a comment with some questions.

adisos · 2024-09-12T05:34:59Z

pkg/vpcmodel/abstractVPC.go

@@ -21,6 +21,7 @@ const (
 type VPCResourceIntf interface {
 	UID() string
 	Name() string
+	NameForAnalyzerOut() string


still confusing/unclear functionality for Name / NameForAnalyzerOut / ExtendedName / ExtendedPrefix.

Does the interface need to have all these methods?

what is the use case for each method?

which methods are overriden for certain types? is this required? (cannot be implemented explicitly instead of changing expected behavior of certain method?)

is there a method that always returns the original resource name from the config object?

is there a method that always returns the original resource name from the config object?

Name()
consider renaming to resourceName() + doc

Does the interface need to have all these methods?

we need Name() and NameForAnalyzerOut , ExtendedName and ExtendedPrefix will check if we can merge it with NameForAnalyzerOut

adisos

looks good!
few more comments

adisos · 2024-09-23T19:33:04Z

pkg/ibmvpc/vpc.go

+	prefix := ""
+	if c != nil && c.IsMultipleVPCsConfig {
+		prefix = pip.VPC().Name() + vpcmodel.Deliminator
+	}


this code seems to be duplicated across this file.. can be moved to some other internal function that gets config and VPC name as inputs?

adisos · 2024-09-23T19:36:21Z

pkg/vpcmodel/abstractVPC.go

-	return n.ExtendedPrefix(c) + n.Name()
+func (n *VPCResource) NameForAnalyzerOut(c *VPCConfig) string {
+	prefix := ""
+	if c != nil && c.IsMultipleVPCsConfig {


maybe the config type should have a method that encapsulates this concept of required prefix?

adisos · 2024-09-23T19:39:15Z

one more comment: can you revisit where NameForAnalyzerOut is called with nil and where not?
when is it required? when it can be assumed it is not needed?
are there places where the config can actually be retrieved but currently not used in the call? and if so, why?

olasaadi99 · 2024-09-24T10:19:29Z

one more comment: can you revisit where NameForAnalyzerOut is called with nil and where not? when is it required? when it can be assumed it is not needed? are there places where the config can actually be retrieved but currently not used in the call? and if so, why?

I assumed that it is not needed where we originally called the previous func without the config... and passed the config where we originaly called ExtendedName (with the config).

adisos · 2024-09-24T11:04:13Z

one more comment: can you revisit where NameForAnalyzerOut is called with nil and where not? when is it required? when it can be assumed it is not needed? are there places where the config can actually be retrieved but currently not used in the call? and if so, why?

I assumed that it is not needed where we originally called the previous func without the config... and passed the config where we originaly called ExtendedName (with the config).

makes sense, but if you can revisit as suggested above this may help to better understand these aspects, update documentation as to where this is expected to be used, etc.. (maybe also find if it makes sense to add the config in other places, to be less error-prone).

when should call NameForAnalyzerOut with config arg (not nil)?
debug messages are also part of "output".. where a more accurate string name is provided.
I think it is best to add where possible, for both consistency and accuracy.

adisos · 2024-09-24T11:06:35Z

pkg/ibmvpc/vpc.go

+	if c != nil {
+		prefix = c.MultipleVPCsConfigPrefix(r.VPC().Name())


can add another internal function that gets as input arg the config object and returns that prefix, and here always just have prefix + nameWithBracketsInfo(r.vpe, r.Address())
(to avoid this duplication across this file as well) .

adisos · 2024-09-24T11:07:35Z

pkg/vpcmodel/vpcConfig.go

@@ -44,6 +44,14 @@ type VPCConfig struct {
 	IsMultipleVPCsConfig bool
 }

+// MultipleVPCsConfigPrefix returns the passed prefix when config is multi-vpc
+func (c *VPCConfig) MultipleVPCsConfigPrefix(prefix string) string {


change arg name to vpcName ?

adisos · 2024-09-24T13:36:23Z

pkg/commonvpc/vpc.go

+
+// MultipleVPCsConfigPrefix returns the passed vpcName when config is multi-vpc
+func MultipleVPCsConfigPrefix(c *vpcmodel.VPCConfig, vpcName string) string {
+	if c != nil && c.IsMultipleVPCsConfig {


Suggested change

if c != nil && c.IsMultipleVPCsConfig {

if c != nil {

adisos

looks good.
There are more cases with NameForAnalyzerOut(nil) is used. Do you have insights about those cases?

adisos · 2024-09-24T19:03:44Z

pkg/commonvpc/vpc.go

+
+// MultipleVPCsConfigPrefix returns the vpcName of the passed resource when config is multi-vpc
+func MultipleVPCsConfigPrefix(c *vpcmodel.VPCConfig, resource *vpcmodel.VPCResource) string {
+	if c != nil && resource.VPC() != nil {


when is resource.VPC() expected to be nil?

I got some tests failed before I added this condition, it seems like there are some cases we do not init config file in tests

only unit tests or end-to-end tests?
this is not about initializing config file, but having the resource a pointer to the VPC it is contained in.

olasaadi99 · 2024-09-25T12:19:35Z

looks good. There are more cases with NameForAnalyzerOut(nil) is used. Do you have insights about those cases?

in these cases the config param is not reachable, or that we need to make a lot of changes

adisos · 2024-09-25T13:44:38Z

looks good. There are more cases with NameForAnalyzerOut(nil) is used. Do you have insights about those cases?

in these cases the config param is not reachable, or that we need to make a lot of changes

@ShiriMoran can you review the places where NameForAnalyzerOut(nil) is now used in this PR, and whether it is safe to keep these calls, or whether the full name (with vpc prefix for multi-vpc cases) should be used instead? (such as explainability output)

ShiriMoran · 2024-09-29T06:15:14Z

pkg/vpcmodel/abstractVPC.go

-	return n.ExtendedPrefix(c) + n.Name()
+func (n *VPCResource) NameForAnalyzerOut(c *VPCConfig) string {
+	prefix := ""
+	if c != nil && n.VPC() != nil {


if c != nil, can n.VPC() be nil?

ShiriMoran · 2024-09-29T06:43:49Z

@ShiriMoran can you review the places where NameForAnalyzerOut(nil) is now used in this PR, and whether it is safe to keep these calls, or whether the full name (with vpc prefix for multi-vpc cases) should be used instead? (such as explainability output)

Syntactic and backwards compatibility wise, it is safe. Namely, all the places in which NameForAnalyzerOut(nil) are places in which Name() was used before
There are places in which Name() was used before and NameForAnalyzerOut(c) is called now, which is good
This leaves us in some mode of confusion, in which there seems to be no rule, or one that makes sense for a user, as to when we avoid the relevant VPC prefix
The above rule is "places where before we used Name() in functions that do not have the VPC config". I think we should avoid this as much as possible, namely avoid calling NameForAnalyzerOut with nil.
Thus, we should go over all the cases in which NameForAnalyzerOut(nil) is called and try to replace it with NameForAnalyzerOut(c); this will require adding vpc config parm to the relevant functions. This can be done in a followup PR @adisos @olasaadi99 (or in this one, what ever you prefer)

adisos · 2024-09-29T07:34:10Z

@ShiriMoran can you review the places where NameForAnalyzerOut(nil) is now used in this PR, and whether it is safe to keep these calls, or whether the full name (with vpc prefix for multi-vpc cases) should be used instead? (such as explainability output)

1. Syntactic and backwards compatibility wise, it is safe. Namely, all the places in which `NameForAnalyzerOut(nil)` are places in which `Name()` was used before

2. There are places in which  `Name()` was used before and `NameForAnalyzerOut(c)` is called now, which is good

3. This leaves us in some mode of confusion, in which there seems to be no rule, or one that makes sense for a user, as to when we avoid the relevant `VPC` prefix

4. The above rule is "places where before we used `Name()` in functions that do not have the `VPC config`". I think we should avoid this as much as possible, namely avoid calling `NameForAnalyzerOut` with `nil`.

5. Thus, we should go over all the cases in which  `NameForAnalyzerOut(nil)` is called and try to replace it with  `NameForAnalyzerOut(c)`; this will require adding `vpc config` parm to the relevant functions. This can be done in a followup PR @adisos @olasaadi99 (or in this one, what ever you prefer)

Thanks @ShiriMoran , sounds good. I think so as well.
@olasaadi99 please check if you prefer a separate PR for this.

olasaadi99 · 2024-09-29T10:53:56Z

thanks
prefer new pr
#892

olasaadi99 added 2 commits September 10, 2024 12:00

refactor Name() and ExtendedName(c) methods

ced445d

fix lint

e87957c

olasaadi99 requested a review from adisos September 10, 2024 09:06

pull-request-size bot added the size/L label Sep 10, 2024

olasaadi99 linked an issue Sep 10, 2024 that may be closed by this pull request

Refactor methods for VPC resource names interfaces #590

Closed

olasaadi99 and others added 2 commits September 10, 2024 12:06

Merge branch 'main' into 590_refactor

d9e0a6a

fix merge

a9be1c9

adisos requested changes Sep 12, 2024

View reviewed changes

olasaadi99 and others added 5 commits September 22, 2024 12:43

Merge branch 'main' into 590_refactor

f9bbd6d

remove extendedPrefix method

ca4cef8

fixes

4449b15

Merge branch 'main' into 590_refactor

ded85a8

remove ExtendedName() method

b152050

olasaadi99 requested a review from adisos September 23, 2024 10:00

Merge branch 'main' into 590_refactor

a6275ee

adisos reviewed Sep 23, 2024

View reviewed changes

fix

0f9482f

adisos reviewed Sep 24, 2024

View reviewed changes

fix

ffe3b88

adisos reviewed Sep 24, 2024

View reviewed changes

olasaadi99 added 4 commits September 24, 2024 19:26

fix tests

af7f353

fix

15cc8ee

pass config param where possible

c9cb9cb

fix

c87a8bc

adisos reviewed Sep 24, 2024

View reviewed changes

Merge branch 'main' into 590_refactor

03d3716

adisos requested a review from ShiriMoran September 25, 2024 13:45

Merge branch 'main' into 590_refactor

36d48db

ShiriMoran reviewed Sep 29, 2024

View reviewed changes

adisos approved these changes Sep 29, 2024

View reviewed changes

olasaadi99 merged commit 7497375 into main Sep 29, 2024
4 checks passed

olasaadi99 deleted the 590_refactor branch September 29, 2024 11:12

		if c != nil {
		prefix = c.MultipleVPCsConfigPrefix(r.VPC().Name())

refactor Name() and ExtendedName(c) methods #865

refactor Name() and ExtendedName(c) methods #865

Uh oh!

Conversation

olasaadi99 commented Sep 10, 2024

Uh oh!

adisos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisos commented Sep 23, 2024

Uh oh!

olasaadi99 commented Sep 24, 2024

Uh oh!

adisos commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olasaadi99 commented Sep 25, 2024

Uh oh!

adisos commented Sep 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShiriMoran commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisos commented Sep 29, 2024

Uh oh!

olasaadi99 commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adisos commented Sep 24, 2024 •

edited

Loading

ShiriMoran commented Sep 29, 2024 •

edited

Loading

olasaadi99 commented Sep 29, 2024 •

edited

Loading