Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add apt credentials parser #60
Add apt credentials parser #60
Changes from 13 commits
f573944
88c1fb0
cfcc048
ff67466
023a9a1
aea488e
a803cf3
15afa40
8be5a54
518af57
8c109e3
959c8c2
c6db9e7
552b996
dc4f7d9
715a51c
5f4627f
b50901e
010e969
5e335b1
482ff15
db609e1
ddefc45
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Per the other comment in the fake function, this should be a global:
This will simplify the logic below.
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.
If I simplify the logic below, then the test will work differently when
CHISEL_AUTH_DIR
env var is set. To make this change I need another fakeEnv call in the test.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.
I have just removed the fake function and test
FindCredentialsInDir
directly.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.
Per earlier comments, the default path should be a global variable, or a constant, according to your preference, and the logic must be tested according to these notes too. This function, and in particular the use of CHISEL_AUTH_DIR, looks untested?
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.
I've moved the constant to the global scope and added FindCredentials test. @niemeyer Please resolve this thread if appropriate.
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.
Just an observation: here we are concluding that no credentials exist, which could be reached if
CHISEL_AUTH_DIR
is set, but pointing to a non-existing directory. This may be by design, but just pointing out that this does not mean that/etc/apt/auth.conf.d
does not contain credentials.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.
It is indeed by my design. :-) We did not specify this, so I did what felt intuitive to me. Warning when
CHISEL_AUTH_DIR
doesn't exist and ignoring non-existent/etc/apt/auth.conf.d
feels inconsistent to me. Would you like a different behavior?Example from elsewhere:
mpv
looks up its configs in$MPV_HOME
or in~/.config/mpv/
(among other locations). It doesn't complain if either of those doesn't exist. Similarly,git
doesn't complain when$GIT_CONFIG_SYSTEM
or/etc/gitconfig
doesn't exist but it reads it if it does. In both cases, if the variable is set (or non-empty?), the default location is skipped.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.
I see both of your points. I think it would help to simply have a debug log message to say that
CHISEL_AUTH_DIR
's path does not exist, regardless of its path.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.
It does make sense to me, the naming of the environment variable suggests an overriding behaviour, which if abused, must yield consequences :)
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.
@cjdcordeiro @flotter I've added a debug message when the credentials directory does not exist, regardless of whether it's the default location or overridden by the variable. See commit 482ff15. Please resolve if you think it addresses your 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.
As a good rule of thumb, any function that is requested to perform a given task must either succeed at its task, or return an error informing that it could not. That's even more true when the function actually returns an error, as the caller will imply that unless there's an error, its intention was successful. The task of this function is to find credentials in a directory, so it must either succeed at that, or return an error informing that it cannot perform its requested task.
The call site, though, may choose to ignore a given scenario. For handling some of those cases we have error results that can be more easily verified, either directly or via a function.
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.
Introduced
ErrCredentialsNotFound
error. The function now returns this error when no credentials are found. @niemeyer Please resolve the thread if appropriate.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.
This needs to be improved a bit so that the intention is more clear. If the point here is that we already have credentials, we should check creds explicitly instead of checking query and assuming credentials are set in 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.
Replaced with
!creds.Empty()
. I can't rely onerr != nil
here as that alone does not tell me whether the URL contained credentials. @niemeyer Please resolve the thread if appropriate.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.
What's the intention here? Why is this not just
var errs []error
?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.
As part of the change to return
ErrCredentialsNotFound
when no credentials are found, non-fatal errors are now logged instead so the function no longer collects them, and this variable was removed. @niemeyer Please resolve the thread if appropriate.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.
Similar to the earlier point on errors here: the loop above may skip all the way through, and we end up with no credentials and no error either.
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.
As I commented above, I've introduced ErrCredentialsNotFound error. The function now returns this error when no credentials are found. @niemeyer Please resolve the thread if appropriate.
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.
Let's please have a more clear interface on findCredsInFile, making it return the credentials when it finds them, as usual in Go, instead of having C-style output parameters unnecessarily. We should also not have to test creds for being empty. Per earlier notes, if the error is nil, the intent of the function must have been fulfilled.
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.
As I commented above, I've introduced
ErrCredentialsNotFound
error. The functionfindCredsInFile
was renamed tofindCredentialsInternal
and it now returns either a non-nil pointer to credentials or a non-nil error when an error occurs or credentials are not found. @niemeyer Please resolve the thread if appropriate.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.
While we're still here, thanks for the extensive documentation and references above.
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.
Per note above, which of these is input and which of these is output? Let's please clean up the function prototype to reflect that, and have the error result reflect intent, per notes above.
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.
Commented about this function in the above thread. @niemeyer Please resolve the thread if appropriate.
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.
Feels like this error needs checking, as it'd be surprising and error prone for a state to report a problem and it just be ignored.
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.
When
err != nil
thenstate == nil
, the loop breaks and the error is later returned, unless an error happened in scanner, in which case the parser error is consequence of it, and so the scanner error is returned instead. It would be a bug for a state to return bothstate
anderr
non-nil. Does it make sense?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.
that logic is true @woky.
Some questions though:
err != nil
andstate == nil
, then do we need to continue to line 221? If not, better check theerr
here and justreturn
It would be a bug for a state to return both state and err non-nil.
. So isn't that the check you should be doing? I.e.if state != nil && err != nil
?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.
We need to go to 221 because states return error when
scanner.Scan()
returns false which can happens on EOF or on an error condition. States treat the false return as EOF. We need to check if it's not an IO error.Those are two things. The
state != nil
is the algorithm. Thestate != nil && err != nil
is the algorithm AND assertion that checks for bugs. I've added the assertion.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.
So this gets back to the start state? Again, the idea of the parser being in an invalid state and just going on is a bit confusing, and it seems to relate to the bug mentioned earlier as it just ignores the current context while looking for special keywords. We'll probably need to sync on this with more bandwidth.
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.
This is how the format is defined and parsed by Apt. It deliberately ignores current context. We're not at liberty to interpret the file differently.
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.
This looks like it'll match the port ":90" when looking at ":9000".
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 check below mitigates that