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.

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

hostRules: improve matchHost validation and parsing #29425

Closed
rarkins opened this issue Jun 4, 2024 · 7 comments · Fixed by #29487
Closed

hostRules: improve matchHost validation and parsing #29425

rarkins opened this issue Jun 4, 2024 · 7 comments · Fixed by #29487
Assignees
Labels
priority-2-high Bugs impacting wide number of users or very important features type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Jun 4, 2024

Describe the proposed change(s).

Goal: massage "incorrect" matchHost values with ports or paths, for example gitlab.company.com:8080 or registry.company.com/some/path.

Improve validation: if a matchHost value contains " or / then we should add https:// as the prefix and check if it's a valid URL. If it's not then I think it's safe for us to raise a config validation error. If it's valid, then we don't need to migrate it, because some config.js configurations may not be possible to migrate (the value comes from env).

Also improve validation of empty matchHost. If the matchHost key is defined, but the value is undefined, then we should warn. This is commonly caused by empty process.env.SOMETHING being used. We should similar do for empty string "".

Finally, we should enhance the addRule function to do similar logic and prefix the matchHost with https:// if it contains : or /.

@rarkins rarkins added type:feature Feature (new functionality) priority-2-high Bugs impacting wide number of users or very important features labels Jun 4, 2024
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Jun 4, 2024

Improve validation: if a matchHost value contains " or / then we should add https:// as the prefix and check if it's a valid URL.

you mean : or / ?

@rarkins
Copy link
Collaborator Author

rarkins commented Jun 4, 2024

Yes I did

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Jun 5, 2024

Can you confirm these are correct?

gitlab.company.com:8080 -> https://gitlab.company.com:8080
registry.company.com/some/path -> https://registry.company.com/some/path
github.com -> github.com
api.github.com -> api.github.com
https://someurl.com -> https://someurl.com

@rarkins
Copy link
Collaborator Author

rarkins commented Jun 5, 2024

Yes, correct

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Jun 6, 2024

Improve validation: if a matchHost value contains " or / then we should add https:// as the prefix and check if it's a valid URL. If it's not then I think it's safe for us to raise a config validation error. If it's valid, then we don't need to migrate it, because some config.js configurations may not be possible to migrate (the value comes from env)

We only create a migration PR for the repo config, so it should not be an issue if the config.js takes values from the env.

Also improve validation of empty matchHost. If the matchHost key is defined, but the value is undefined, then we should warn.

How should this be detected? Using for...in or hasOwnProperty ?

@rarkins
Copy link
Collaborator Author

rarkins commented Jun 6, 2024

Yes something like that

@RahulGautamSingh
Copy link
Collaborator

Also improve validation of empty matchHost. If the matchHost key is defined, but the value is undefined, then we should warn.

We already error on this as the matchHost.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-2-high Bugs impacting wide number of users or very important features type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants