Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

wrong name regex to filter url when provide --name=www as parameter in cli#14575

Merged
bkchr merged 1 commit intoparitytech:masterfrom
alt-research:fix/name-regex
Jul 14, 2023
Merged

wrong name regex to filter url when provide --name=www as parameter in cli#14575
bkchr merged 1 commit intoparitytech:masterfrom
alt-research:fix/name-regex

Conversation

@atenjin
Copy link
Contributor

@atenjin atenjin commented Jul 14, 2023 •
edited
Loading

close #14574

In this pr, we just filter http:// https:// prefix in the string, not including word www

@bkchr bkchr added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 14, 2023
@bkchr bkchr requested a review from a team July 14, 2023 09:17
@michalkucharczyk michalkucharczyk requested a review from a team July 14, 2023 09:25
koute
koute approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

Technically URLs don't necessarily have to be for HTTP(S), so maybe we should just disallow URLs for any protocol?

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jul 14, 2023

Technically URLs don't necessarily have to be for HTTP(S), so maybe we should just disallow URLs for any protocol?

had the same thought

@bkchr bkchr merged commit 5bbdcfd into paritytech:master Jul 14, 2023
@atenjin
Copy link
Contributor Author

atenjin commented Jul 15, 2023

@michalkucharczyk hey another thing, can this hotfix pr cherry pick to polkadot-0.9.43 branch? oh maybe this contains in polkadotv1.0.0 is also ok

@bkchr
Copy link
Member

bkchr commented Jul 15, 2023

@atenjin I don't see that this is a critical change that needs to be backported

@atenjin
Copy link
Contributor Author

atenjin commented Jul 16, 2023

@bkchr sorry in our project, we are using this substrate branch in our chain, we meet this bug in our running service.
We can not ensure the time for the next polkadot version, so if this can be moved to that branch is more better

@bkchr
Copy link
Member

bkchr commented Jul 16, 2023

Just don't remove the www from the name of your node. These names are only required for telemetry and nothing else and telemetry isn't important.

@andresilva andresilva mentioned this pull request Jul 17, 2023
Merged
ggwpez
ggwpez reviewed Jul 17, 2023
View reviewed changes
client/cli/src/commands/run_cmd.rs
}

let invalid_patterns = r"(https?:\\/+)?(www)+";
let invalid_patterns = r"^https?:\/\/";
Copy link
Member

@ggwpez ggwpez Jul 17, 2023

Choose a reason for hiding this comment

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

let invalid_patterns = r"^https?:\\/\\/";

Instead?

Copy link
Contributor

@andresilva andresilva Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you don't need to escape the /, so just r"^https?://" should work. Also none of the tests would catch this since the URLs also include a . so this condition is never tested. We could also just filter with r"^https?:/". A name like https:/myfunnywebsite.com is probably something we don't want either.

Copy link
Contributor

@andresilva andresilva Jul 17, 2023

Choose a reason for hiding this comment

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

#14593

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@atenjin @nathanwhit
do not filter word www in is_node_name_valid check (paritytech#14575
0970473 
)
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
@atenjin @Ank4n
do not filter word www in is_node_name_valid check (#14575)
9910c89
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

6 more reviewers

@andresilva andresilva andresilva left review comments

@ggwpez ggwpez ggwpez left review comments

@koute koute koute approved these changes

@melekes melekes melekes approved these changes

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@bkchr bkchr bkchr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

wrong name regex to filter url when provide --name=www as parameter in cli

7 participants

@atenjin @michalkucharczyk @bkchr @andresilva @koute @melekes @ggwpez