-
-
Notifications
You must be signed in to change notification settings - Fork 763
feat: redact credentials in remote urls #2220
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
Conversation
|
@iwittkau, very open to comments as you were the author of the last PR. |
94ff7fa to
474eec1
Compare
|
I think this test should be added an pass: func TestGitNode_redaction(t *testing.T) {
t.Parallel()
node, err := NewGitNode("https://git:[email protected]/foo/bar.git//directory/Taskfile.yml?ref=main", "", false)
assert.NoError(t, err)
assert.Equal(t, "main", node.ref)
assert.Equal(t, "directory/Taskfile.yml", node.path)
assert.Equal(t, "https://git:[email protected]/foo/bar.git//directory/Taskfile.yml?ref=main", node.Location())
// assert.Equal(t, "https://git:[email protected]/foo/bar.git", node.URL.String())
entrypoint, err := node.ResolveEntrypoint("common.yml")
assert.NoError(t, err)
assert.Equal(t, "https://git:[email protected]/foo/bar.git//directory/common.yml?ref=main", entrypoint)
} |
iwittkau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 🎉
I'll give it a try with my work setup tomorrow.
|
I think you could keep GitNode.URL unexported to make sure its |
Yeah, we don't really need any of the properties on |
|
@pd93 I just noticed that this branch needs a |
vmaerten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is cache naming is way better like this!
vmaerten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ! gj :)
|
I've been using this for a while now and it works nicely 👍🏻 |
Fixes #2100 and replaces #2045
Ensures that any credentials embedded in the URL are redacted by using the built in
url.Redacted()function. The cache keys have also been altered slightly, so users may have to redownload some files that they already have cached.