Skip to content

Adds tcp/udp port mapper#35

Merged
itdependsnetworks merged 15 commits into
networktocode:developfrom
jtdub:tcpprotos
Aug 29, 2021
Merged

Adds tcp/udp port mapper#35
itdependsnetworks merged 15 commits into
networktocode:developfrom
jtdub:tcpprotos

Conversation

@jtdub

@jtdub jtdub commented Aug 7, 2021

Copy link
Copy Markdown
Contributor

@jeffkala jeffkala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to sort this? Obviously that doesn't change the code much, but thinking for readability of the code.

@itdependsnetworks

Copy link
Copy Markdown
Contributor

Yes, I agree. I guess sort by number?

@jtdub jtdub requested a review from jeffkala August 9, 2021 18:58

@qduk qduk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this will definitely be useful. Thanks for the PR!

@jeffkala jeffkala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good now. Thanks for sorting it.

@jtdub

jtdub commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

You're welcome. :)

@itdependsnetworks

Copy link
Copy Markdown
Contributor

Actually, what did you do when there was a different tcp vs udp?

doip-data | 13400 | tcp | DoIP Data
doip-disc | 13400 | udp | DoIP Discovery

@jtdub

jtdub commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

@itdependsnetworks

I didn't take that into account, but I can!


import csv

TCP_PORT = {}
TCP_NUM_TO_NAME = {}

with open("service-names-port-numbers.csv") as f:
    reader = csv.reader(f)
    for row in reader:
        if row[0] and row[1]:
            try:
                key_exists = TCP_PORT.get(row[1])
                if not key_exists:
                    TCP_PORT[int(row[1])] = row[0].upper()
            except ValueError:
                continue

for k in sorted(TCP_PORT):
    v = TCP_PORT[k]
    TCP_NUM_TO_NAME[k] = v

print(TCP_NUM_TO_NAME)

@jtdub

jtdub commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

I broke out the individual protocols per @itdependsnetworks catch.

Comment thread pyproject.toml Outdated
@jeffkala jeffkala mentioned this pull request Aug 18, 2021
@jeffkala

Copy link
Copy Markdown
Collaborator

Didn't realize pkg_resources was in the standard library, so this is all fixed now. I just checkout'd out the PR and ran the tests and it works.

jeffkala
jeffkala previously approved these changes Aug 27, 2021

@jeffkala jeffkala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good now, tested it locally and seems to work as expected.

@jeffkala

Copy link
Copy Markdown
Collaborator

Didn't realize pkg_resources was in the standard library, so this is all fixed now. I just checkout'd out the PR and ran the tests and it works.

Realizing this is not actually the case, appears pip installs this. When i setup a new venv without pip this isn't importable.

@jeffkala jeffkala dismissed their stale review August 27, 2021 21:12

not approved now that pkg_resources is not std lib

@jeffkala jeffkala self-requested a review August 27, 2021 21:13

@jeffkala jeffkala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Think this gets us back to the original solution to just load the JSON file @jtdub , this is what we'd prefer to avoid adding dependencies.

Update as needed if I missed anything.

Comment thread netutils/constants.py Outdated
Comment thread netutils/constants.py Outdated
@jtdub jtdub requested a review from jeffkala August 28, 2021 22:22
@itdependsnetworks itdependsnetworks merged commit f5317af into networktocode:develop Aug 29, 2021
@itdependsnetworks

Copy link
Copy Markdown
Contributor

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants