-
Notifications
You must be signed in to change notification settings - Fork 35
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
How to add exception to allow e.g. git+ssh://git@gitlab.foo.bar
#11
Comments
That's a good point. What would be the use-case we'd want to support? a specific one-off complete URL as a source? such as something like |
I would think that in order to support public repositories such as github.com, we'd want to validate against the whole URI, optionally including the commit hash or branch name, such as you suggest: |
so I'm thinking you could whitelist such URLs in this way: If you only want to lint based on the hostname:
notice that with the change I'm thinking about, If you want to lint based on the hostname and enable https
WDYT? |
That sounds like an improvement. I have a couple suggestions:
There would need to be some way of grouping these in the commandline interface to correspond to the relevant allowed-host. Here's an example where three hosts are allowed (yarn, github.com, and gitlab.com) but the last two are restricted by
NotesI think
Specifying an |
I have actually aligned with the syntax used by Node.js's URL API: https://nodejs.org/api/url.html#url_url_protocol - though perhaps worth to use the universal language for it instead of the object keys. Indeed not sure how authority and fragment will only be relevant for specific schemes so I'm not 100% on those. Agree with I actually had to make a change though, since right now we're validating the |
@nottoseethesun could you provide some practical examples of installing from git and how does that look in a package-lock.json and in a yarn.lock files? I think looking at the resolved entry would not suffice |
On the I agree on As a really simple way to offer the flexibility we're looking for here in terms of letting developers use this tool even on projects that have, for example, dep's on github.com projects, perhaps the commandline option could support something like, |
but then for p.s. appreciate all the feedback ❤️ |
Regarding your question about a practical example, here are a couple:
|
Yep, so looks like for yarn we can go with |
So I was just thinking that the |
Alrighty, let's leave out allowed-uri for now as I think we have a plan around allowed schemes and allowed hosts updates and that should work well for now. We can work out allowed uri in future PR. |
I would like to see
|
so @bolatovmar suggested this https://github.com/lirantal/lockfile-lint/pull/52/files#diff-803bbd31414b876c6a30b1dcf035cac8R44 which essentially "upgrades' the
I have some observations on that PR if we want to land that kind of change:
cc @ixti @nottoseethesun appreciate your input as you have both been active in suggesting support for this. |
I'm fine either way. :D It's just |
Ok. What do you mean by |
@lirantal I mean when I see argument name |
Yep, I was somewhat concerned with that too which is why I hesitated on merging it until now. |
@bolatovumar I have an idea on how we can support a dedicated command line parameter here. Would you be open and available to making the changes in your PR so we can merge it? |
@lirantal sorry, was a bit busy recently so didn't see this. I might have some time in a week or so to adjust my PR to support a dedicated command line parameter. Let me know what you are thinking. |
Sure thing. Details on the PR page as I saw you commented there |
Addressed in #52 |
For
--allowed-hosts
, how to allow also a specificgit+ssh://git@gitlab.foo.bar
source?This would also have to work with the option to require https.
The text was updated successfully, but these errors were encountered: