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

chore(internet_port): added new ports and removed unnecessary string class #27078

Merged
merged 8 commits into from
Feb 15, 2024
Merged

chore(internet_port): added new ports and removed unnecessary string class #27078

merged 8 commits into from
Feb 15, 2024

Conversation

anirudh-hegde
Copy link
Contributor

@john-bodley
Copy link
Member

Thanks @anirudh-hegde for the change. @cccs-tom would you mind taking a look at this?

cccs-tom
cccs-tom approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@cccs-tom cccs-tom left a comment

Choose a reason for hiding this comment

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

Lgtm!

villebro
villebro approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@villebro villebro changed the title improved [internet_port.py] : added new ports and removed unnecessary string class chore(internet_port): added new ports and removed unnecessary string class Feb 12, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4796484) 67.18% compared to head (3ba27e5) 69.50%.
Report is 8 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #27078      +/-   ##
==========================================
+ Coverage   67.18%   69.50%   +2.32%     
==========================================
  Files        1900     1900              
  Lines       74443    74433      -10     
  Branches     8293     8293              
==========================================
+ Hits        50012    51733    +1721     
+ Misses      22376    20645    -1731     
  Partials     2055     2055
Flag Coverage Δ
hive 53.80% <0.00%> (?)
mysql 78.02% <0.00%> (-0.01%) ⬇️
postgres 78.12% <0.00%> (-0.01%) ⬇️
presto 53.75% <0.00%> (?)
python 83.08% <100.00%> (+4.82%) ⬆️
sqlite 77.64% <0.00%> (-0.01%) ⬇️
unit 56.42% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@villebro
Copy link
Member

@anirudh-hegde please check the linting errors:
image

@villebro
Copy link
Member

@anirudh-hegde there's still that trailing whitespace issue:

superset/advanced_data_type/plugins/internet_port.py:93:0: C0303: Trailing whitespace (trailing-whitespace)

@anirudh-hegde
Copy link
Contributor Author

image
It means in line 91 - 93 there is a trailing whitespace?

villebro
villebro reviewed Feb 13, 2024
View reviewed changes
superset/advanced_data_type/plugins/internet_port.py Outdated
f"'{string_value}' does not appear to be a port name or number"
)

Copy link
Member

Choose a reason for hiding this comment

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

This PR adds a newline here which the linter wants to be removed

Suggested change

@anirudh-hegde
Copy link
Contributor Author

I used pylint internet_port.py
image

@anirudh-hegde
Copy link
Contributor Author

Ohh my bad!! I merged branch 'apache:master' into spear

villebro
villebro reviewed Feb 13, 2024
View reviewed changes
superset/advanced_data_type/plugins/internet_port.py Outdated
"Remote Desktop": [3389],
"dns": [53],
"ctf": [84],
"pdap": [344]
Copy link
Member

Choose a reason for hiding this comment

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

This is still failing:

Suggested change
"pdap": [344]
"pdap": [344],

@anirudh-hegde
Copy link
Contributor Author

Should I need to change the code?

@rusackas
Copy link
Member

Looks like you just need to run the pre-commit hook and you should be OK.

@anirudh-hegde
Copy link
Contributor Author

Looks like you just need to run the pre-commit hook and you should be OK.

Hey @rusackas,
image
I used pre-commit run --all-files

@anirudh-hegde
Copy link
Contributor Author

Looks like you just need to run the pre-commit hook and you should be OK.

You were right.
Thank you, @rusackas

@rusackas rusackas merged commit 3224ac1 into apache:master Feb 15, 2024
40 of 41 checks passed
@anirudh-hegde anirudh-hegde deleted the spear branch February 15, 2024 06:08
@anirudh-hegde anirudh-hegde restored the spear branch February 15, 2024 06:08
@anirudh-hegde anirudh-hegde deleted the spear branch February 15, 2024 06:08
grvoicu added a commit to grvoicu/superset that referenced this pull request Feb 20, 2024
@grvoicu
Merge branch 'master' into feat/embedded-pass-static-query-parameters…
31a06d1 
…-to-the-embedded-dashboard

* master: (1182 commits)
  fix(ci): mypy pre-commit issues (apache#27161)
  feat(Alerts and Reports): Modal redesign (apache#26202)
  refactor: Migrate ErrorBoundary to typescript (apache#27143)
  chore(tests): Remove unnecessary explicit Flask-SQLAlchemy session expunges (apache#27136)
  fix(plugins): Apply dashboard filters to comparison query in BigNumber with Time Comparison chart (apache#27138)
  fix: Duplicated toast messages (apache#27135)
  docs: add Geotab to users list (apache#27134)
  fix: Plain error message when visiting a dashboard via permalink without permissions (apache#27132)
  fix: ID param for DELETE ssh_tunnel endpoint (apache#27130)
  chore(hail mary): Update package-lock.json via npm-audit-fix (apache#26693)
  chore: lower cryptography min version to 41.0.2 (apache#27129)
  docs(miscellaneous): Export Datasoruces: export datasources exports to ZIP (apache#27120)
  fix(pivot-table-v2): Added forgotten translation pivot table v2 (apache#22840)
  fix: RLS modal overflow (apache#27128)
  refactor: Updates some database columns to MediumText (apache#27119)
  fix: gevent upgrade to 23.9.1 (apache#27112)
  fix: removes old deprecated sqllab endpoints (apache#27117)
  feat(storybook): Co-habitating/Upgrading Storybooks to v7 (dependency madness ensues) (apache#26907)
  fix: bump grpcio, urllib3 and paramiko (apache#27124)
  chore(internet_port): added new ports and removed unnecessary string class (apache#27078)
  ...
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@anirudh-hegde @sfirke
chore(internet_port): added new ports and removed unnecessary string …
d33e71c 
…class (apache#27078)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@villebro villebro villebro approved these changes

@cccs-tom cccs-tom cccs-tom approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 4.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@anirudh-hegde @john-bodley @villebro @rusackas @cccs-tom @mistercrunch