Lower & upper IP tool #155

Kashaf-Sajid-Yaseen · 2023-07-24T17:45:18Z

Updated cmd_tool.go file in ipinfo folder. Additionally created cmd_tool_lower.go & cmd_tool_upper.go files in ipinfo and lib folders. To get start and end IP addresses of input IP entered by the user.

awaismslm

There is a lot of useless code. Please optimize the code.

awaismslm

Auto complete is also not working. Please check it too

ipinfo/cmd_tool_lower.go

ipinfo/cmd_tool_upper.go

awaismslm · 2023-07-26T08:39:49Z

lib/cmd_tool_lower.go

+
+import (
+	"fmt"
+


Suggested change

lib/cmd_tool_lower.go

awaismslm · 2023-07-26T08:47:56Z

lib/common.go

@@ -0,0 +1,35 @@
+package lib


Please rename this file. You might name it utils.go. we can add more utility functions there in future.

As these functions are tool related So its better to move them in cmd_tool.go
(Please ignore above)

awaismslm · 2023-07-26T08:48:26Z

lib/common.go

+			return err
+		}
+	}
+


Suggested change

lib/cmd_tool_lower.go

lib/cmd_tool_upper.go

lib/cmd_tool_lower.go

awaismslm

Great work. We are almost done. Just a few things.
Also, please update the printHelp of cmd_tool to include the help message of upper and lower.

awaismslm · 2023-07-31T06:50:18Z

ipinfo/build/ipinfo

We do not commit the binaries. Please remove it.

lib/cmd_tool_lower.go

awaismslm · 2023-07-31T06:52:44Z

lib/cmd_tool_upper.go

+		printHelp()
+		return nil
+	}
+	stat, _ := os.Stdin.Stat()


Suggested change

stat, _ := os.Stdin.Stat()

stat, _ := os.Stdin.Stat()

awaismslm · 2023-07-31T06:52:59Z

lib/cmd_tool_upper.go

+	}
+	stat, _ := os.Stdin.Stat()
+	isStdin := (stat.Mode() & os.ModeCharDevice) == 0
+	if len(args) == 0 && !isStdin {


Suggested change

if len(args) == 0 && !isStdin {

if len(args) == 0 && !isStdin {

awaismslm · 2023-07-31T06:54:06Z

lib/cmd_tool_upper.go

+			if !f.Quiet {
+				fmt.Printf("Error parsing CIDR: %v\n", err)
+			}
+			return nil


shouldn't we return the error instead of nil?

awaismslm · 2023-07-31T06:58:31Z

ipinfo/ipinfo

same, no binaries

awaismslm

Just a small change and lets do the final review.

awaismslm · 2023-07-31T09:53:07Z

ipinfo/cmd_tool.go

@@ -26,7 +28,8 @@ func printHelpTool() {

 Commands:
  aggregate    aggregate IPs, IP ranges, and CIDRs.
-
+  lower        lower CIDRs(return the starting IP addresses of entered input).


Suggested change

lower lower CIDRs(return the starting IP addresses of entered input).

lower get start IP of CIDR

awaismslm · 2023-07-31T09:53:36Z

ipinfo/cmd_tool.go

@@ -26,7 +28,8 @@ func printHelpTool() {

 Commands:
  aggregate    aggregate IPs, IP ranges, and CIDRs.
-
+  lower        lower CIDRs(return the starting IP addresses of entered input).
+  upper        upper CIDRs(return the ending IP addresses of entered input).


Suggested change

upper upper CIDRs(return the ending IP addresses of entered input).

upper get end IP of CIDR

awaismslm · 2023-07-31T10:36:07Z

@UmanShahzad Please do the final review. I am done from my side.

UmanShahzad

Why did we only accept CIDRs and not ranges too? #113 says ranges too.

Also this doesn't seem generic enough from the input side still. See lib/ip_list_write.go#IPListWriteFrom to understand the level of abstraction we need here. You can take that function and create an analogue that writes the lower/upper IPs. What's even better than copy/paste though which I would much more prefer, is creating a generic version of that function which accepts a function as input that will do some generic action on the IPs; then we could use this for all of our use cases here, even the original one in IPListWriteFrom.

ipinfo/cmd_tool.go

lib/cmd_tool_lower.go

lib/cmd_tool_upper.go

Kashaf-Sajid-Yaseen · 2023-08-02T17:12:14Z

@UmanShahzad Please check

UmanShahzad

This isn't using the generic algorithm as explained above

ipinfo/cmd_tool.go

UmanShahzad · 2023-08-03T09:18:03Z

ipinfo/cmd_tool.go

@@ -27,6 +29,8 @@ func printHelpTool() {
 Commands:
  aggregate    aggregate IPs, IP ranges, and CIDRs.

+  lower        get start IP of IP, IP ranges, and CIDRs.


Suggested change

lower get start IP of IP, IP ranges, and CIDRs.

lower get start IP of IPs, IP ranges, and CIDRs.

UmanShahzad · 2023-08-03T09:18:08Z

ipinfo/cmd_tool.go

@@ -27,6 +29,8 @@ func printHelpTool() {
 Commands:
  aggregate    aggregate IPs, IP ranges, and CIDRs.

+  lower        get start IP of IP, IP ranges, and CIDRs.
+  upper        get end IP of IP, IP ranges, and CIDRs.


Suggested change

upper get end IP of IP, IP ranges, and CIDRs.

upper get end IP of IPs, IP ranges, and CIDRs.

UmanShahzad · 2023-08-03T09:18:34Z

ipinfo/cmd_tool_lower.go

+
+func printHelpToolLower() {
+	fmt.Printf(
+		`Usage: %s tool lower [<opts>] <cidr>


It's not just <cidr> now

Also we allow files too now right?

UmanShahzad · 2023-08-03T09:18:45Z

ipinfo/cmd_tool_upper.go

+
+func printHelpToolUpper() {
+	fmt.Printf(
+		`Usage: %s tool upper [<opts>] <cidr>


It's not just <cidr> now

UmanShahzad · 2023-08-03T09:20:04Z

ipinfo/cmd_tool_lower.go

+  Input can be IP, IP range or CIDR.
+
+  If input contains CIDRs, it calculates the lower IP address for each CIDR.
+  If input contains IP ranges, it calculates the lower IP address for each Ip range.


We can remove these above 2 description lines, they're redundant

UmanShahzad · 2023-08-03T09:20:24Z

ipinfo/cmd_tool_lower.go

+
+Description:
+  Calculates the lower IP address (start address of a network) for the given inputs.
+  Input can be IP, IP range or CIDR.


Suggested change

Input can be IP, IP range or CIDR.

Inputs can be a mixture of IPs, IP ranges or CIDRs.

UmanShahzad · 2023-08-03T09:20:45Z

ipinfo/cmd_tool_upper.go

+  Input can be Ip, IP range or CIDR.
+
+  If input contains CIDRs, it calculates the upper IP address for each CIDR.
+  If input contains Ip ranges, it calculates the upper IP address for each IP range.


We can remove these above 2 description lines, they're redundant

UmanShahzad · 2023-08-03T09:21:04Z

ipinfo/cmd_tool_upper.go

+
+Description:
+  Calculates the upper IP address (end address of a network) for the given inputs.
+  Input can be Ip, IP range or CIDR.


Suggested change

Input can be Ip, IP range or CIDR.

Inputs can be a mixture of IPs, IP ranges or CIDRs.

Kashaf-Sajid-Yaseen · 2023-08-03T11:39:06Z

Please check @UmanShahzad, I have updated the above files.

UmanShahzad

This is still not right. @awaismslm @Kashaf-Sajid-Yaseen

UmanShahzad · 2023-08-08T14:54:47Z

@Kashaf-Sajid-Yaseen lemme know when ready?

awaismslm

@UmanShahzad Please take a look if the approach is correct. I can see that we have to make the generic functions for actions: not specific to upper and lower. Other than that the approach seems fine. I'll check the other code.

UmanShahzad · 2023-08-10T04:34:35Z

So @Harisabdullah is developing a generic solution for that which he'll use in #159 and which we can use here too. Let us hold out until that is done then we can use that solution here.

I just checked this one and it's not too bad either, but @Harisabdullah will have one with a better interface (the one used here is the one I wrote very rapidly in pseudocode).

UmanShahzad · 2023-08-18T04:17:03Z

@awaismslm / @Kashaf-Sajid-Yaseen you can check the latest abstraction used in #159 for this, and use that for all tools now.

awaismslm · 2023-08-21T06:07:38Z

Looks good to me. @Kashaf-Sajid-Yaseen Please resolve the conflicts.

awaismslm · 2023-08-21T08:54:24Z

@UmanShahzad Please take a look. It looks good to me.

UmanShahzad · 2023-08-21T10:43:26Z

ipinfo/cmd_tool_lower.go

+$ cat /path/to/file.txt | %[1]s tool lower
+
+# Find lower IPs from file.
+$ %[1]s tool lower /path/to/file1.txt 


Need proper spacing for all of these examples (2 chars)

UmanShahzad · 2023-08-21T10:44:23Z

lib/cmd_tool_lower.go

+}
+
+func ActionForIP(input string) {
+	ip := net.ParseIP(input)


It is already in a parsed state and valid - just print it, no need to parse this.

UmanShahzad · 2023-08-21T10:44:52Z

lib/cmd_tool_lower.go

+		fmt.Println(err)
+		return
+	}
+	fmt.Println(ipnet.IP) // Print the start IP of the CIDR


No need for the trailing comment

lib/cmd_tool_lower.go

lib/cmd_tool_upper.go

UmanShahzad · 2023-08-21T10:45:58Z

lib/cmd_tool_upper.go

+		fmt.Println(err)
+		return
+	}
+	fmt.Println(ipRange.End) // Print the end IP of the range


No need for the trailing comment

UmanShahzad · 2023-08-21T10:46:02Z

lib/cmd_tool_upper.go

+		fmt.Println(err)
+		return
+	}
+	fmt.Println(ipRange.End) // Print the end IP of the CIDR


No need for the trailing comment

UmanShahzad · 2023-08-21T10:46:33Z

lib/cmd_tool_upper.go

+}
+
+func ActionForCIDRUpper(input string) {
+	_, ipnet, err := net.ParseCIDR(input)


Just pass the input directly into IPRangeStrFromCIDR, because at this point we know it's a valid CIDR.

UmanShahzad · 2023-08-21T10:46:43Z

lib/cmd_tool_upper.go

+}
+
+func ActionForIPUpper(input string) {
+	ip := net.ParseIP(input)


No need to parse

Kashaf-Sajid-Yaseen · 2023-08-22T08:19:12Z

@UmanShahzad

from newbranch

8a9bcc6

awaismslm suggested changes Jul 25, 2023

View reviewed changes

Code is Updated

134ceac

awaismslm suggested changes Jul 26, 2023

View reviewed changes

Files updated after second Review

b0eabf2

UmanShahzad changed the title ~~from newbranch~~ Lower & upper IP tool Jul 28, 2023

awaismslm suggested changes Jul 31, 2023

View reviewed changes

Files updated after Third Review

beab743

awaismslm approved these changes Jul 31, 2023

View reviewed changes

File updated after fourth Review

73c38da

awaismslm requested a review from UmanShahzad July 31, 2023 11:26

UmanShahzad reviewed Aug 1, 2023

View reviewed changes

ipinfo/cmd_tool.go Outdated Show resolved Hide resolved

lib/cmd_tool_lower.go Show resolved Hide resolved

lib/cmd_tool_upper.go Show resolved Hide resolved

Files updated after Fouth Review

4e7eaae

UmanShahzad suggested changes Aug 3, 2023

View reviewed changes

Files updated after Fifth Review

b6221c4

UmanShahzad reviewed Aug 4, 2023

View reviewed changes

Files updated after Sixth Review

437b62a

awaismslm reviewed Aug 9, 2023

View reviewed changes

Updated with generic function

bf69be3

UmanShahzad reviewed Aug 21, 2023

View reviewed changes

Kashaf-Sajid-Yaseen force-pushed the newbranch branch from b0fdf7d to bf69be3 Compare August 21, 2023 12:10

Updated

692a20b

Merge branch 'master' into newbranch

7b82eae

UmanShahzad approved these changes Aug 22, 2023

View reviewed changes

UmanShahzad merged commit dd5467b into ipinfo:master Aug 22, 2023

This was linked to issues Aug 28, 2023

IP tool: CIDR/range end IP #113

Closed

IP tool: CIDR/range start IP #112

Closed

	if len(args) == 0 && !isStdin {

	if len(args) == 0 && !isStdin {

	lower lower CIDRs(return the starting IP addresses of entered input).
	lower get start IP of CIDR

	upper upper CIDRs(return the ending IP addresses of entered input).
	upper get end IP of CIDR

	lower get start IP of IP, IP ranges, and CIDRs.
	lower get start IP of IPs, IP ranges, and CIDRs.

	upper get end IP of IP, IP ranges, and CIDRs.
	upper get end IP of IPs, IP ranges, and CIDRs.

	Input can be IP, IP range or CIDR.
	Inputs can be a mixture of IPs, IP ranges or CIDRs.


		import (
		"fmt"

Lower & upper IP tool #155

Lower & upper IP tool #155

Conversation

Kashaf-Sajid-Yaseen commented Jul 24, 2023

awaismslm left a comment

Choose a reason for hiding this comment

awaismslm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaismslm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaismslm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaismslm commented Jul 31, 2023

UmanShahzad left a comment

Choose a reason for hiding this comment

Kashaf-Sajid-Yaseen commented Aug 2, 2023

UmanShahzad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kashaf-Sajid-Yaseen commented Aug 3, 2023

UmanShahzad left a comment

Choose a reason for hiding this comment

UmanShahzad commented Aug 8, 2023

awaismslm left a comment

Choose a reason for hiding this comment

UmanShahzad commented Aug 10, 2023

UmanShahzad commented Aug 18, 2023

awaismslm commented Aug 21, 2023

awaismslm commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kashaf-Sajid-Yaseen commented Aug 22, 2023