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

Corrects wrong default path for auth.json in docs #1442

Merged
merged 1 commit into from
Jan 18, 2022
Merged

Corrects wrong default path for auth.json in docs #1442

merged 1 commit into from
Jan 18, 2022

Conversation

svdHero
Copy link
Contributor

@svdHero svdHero commented Jan 12, 2022

According to documentation, the default value for XDG_CONFIG_HOME is $HOME/.config/containers.

Hence, ${XDG_CONFIG_HOME}/containers/auth.json would be expanded to $HOME/.config/containers/containers/auth.json, which is clearly one containers too many. I changed that.

I also removed some obsolete backslashes in the variable name.

@svdHero
Copy link
Contributor Author

svdHero commented Jan 12, 2022

What's wrong with the failing test here? I have only changed a single markdown file. 😢

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 12, 2022

Thanks for the PR!

Please sign the commit, per https://github.com/containers/image/blob/main/CONTRIBUTING.md .

(I haven’t yet reviewed the actual proposed change.)

@@ -11,7 +11,7 @@ on Windows and macOS, at `$HOME/.config/containers/auth.json`.

When searching for the credential for a registry, the following files will be read in sequence until the valid credential is found:
first reading the primary (read/write) file, or the explicit override using an option of the calling application.
If credentials are not present, search in `${XDG\_CONFIG\_HOME}/containers/auth.json`, `$HOME/.docker/config.json`, `$HOME/.dockercfg`.
If credentials are not present, search in `${XDG_CONFIG_HOME}/auth.json`, `$HOME/.docker/config.json`, `$HOME/.dockercfg`.
Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, the backslashes are needed for some kind of obscure md functionality.

I'm sure about this change. As noted in the [documentation]{https://docs.podman.io/en/latest/markdown/podman.1.html?highlight=XDG_CONFIG_HOME#xdg-config-home}, we pull information from $XDG_CONFIG_HOME if it is defined, otherwise, we pull it from $HOME/.config/containers. What gets a bit confusing is by default, $XDG_CONFIG_HOME defaults to $HOME/.config

Given that, I think having the containers in there is correct. @svdHero first of all, TY for the PR. Have you seen this fail for you? Was that the impetus for this change? i.e. have I been reviewing too many things and I'm off-base here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomSweeneyRedHat Thanks for the review.

Concerning the backslashes: If they are needed for some markdown magic, why are they not used for ${XDG_RUNTIME_DIR}, too? See here?
Furthermore, the backslashes are visible in the markdown document AFTER rendering. Which probably is not intended.

Concerning the main issue about the double containers. I probably did not understand it correctly. But like you said yourself, it is a bit confusing. I did not see podman fail, I was just not sure if it picks up my auth.json. How can I verify that it actually has been used when doing a podman login harbor.mycompany.com?

I checked the env vars. They are all empty:

schorsch@box:~$ echo ${XDG_CONFIG_HOME}

schorsch@box:~$ echo ${XDG_DATA_HOME}

schorsch@box:~$ echo ${XDG_RUNTIME_DIR}

schorsch@box:~$

Now I have an auth.json which looks like this:

{
  "credHelpers": {
    "harbor.mycompany.com": "pass"
  }
}

Where would I have to store this file, so that it is picked up by podman? If I understand you correctly, it should be $HOME/.config/containers/auth.json, because ${XDG_CONFIG_HOME} is not set, right?

Maybe it would be good to also add this to the auth.json docs, too? I can do that in another commit while also reverting my false correction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I recall correctly, the backslashes are needed for some kind of obscure md functionality.

I guess they were necessary in a not-code-marked version, where underscores are a syntax for italic type.

I agree that they are incorrect here, and should be removed.


According to documentation, the default value for XDG_CONFIG_HOME is $HOME/.config/containers.

That documentation is a bit confusing, and could be improved. It is correct but it could easily be read to mean that XDG_CONFIG_HOME defaults to $HOME/.config/containers, which is not the case.

The containers-auth.json(5) text is correct as is, comparing it with the pkg/docker/config/config.go implementation.


How can I verify that it actually has been used when doing a podman login harbor.mycompany.com?

podman login --verbose outputs where it wrote the credentials.

As for checking whether they have been found and used, debug-level logs (podman --log-level=debug, skopeo --debug) show where credentials were found. They don’t list files that were checked with no match, that would be interesting to add.


Maybe it would be good to also add this to the auth.json docs, too? I can do that in another commit while also reverting my false correction.

The documentation should continue to refer to XDG_CONFIG_HOME, which is what actually happens, but e.g. a parenthetical “(usually `~/.config/containers/auth.json)” might be quite useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for checking whether they have been found and used, debug-level logs (podman --log-level=debug, skopeo --debug) show where credentials were found. They don’t list files that were checked with no match, that would be interesting to add.

#1445 .

@svdHero
Copy link
Contributor Author

svdHero commented Jan 13, 2022

Thanks for the PR!

Please sign the commit, per https://github.com/containers/image/blob/main/CONTRIBUTING.md .

(I haven’t yet reviewed the actual proposed change.)

This is my first Gtihub PR ever. Is it save to rewrite history in order to sign-off the already existing commit? I would do a git commit --amend --signoff.
Also, can / should I squash all commits into one using rebase, even though the commit above is already pushed?

Edit: Oh no. I made it even more complicated now by pressing "Update Branch". 🙈

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2022

Yes that is the way we prefer, one signed PR.

I am pretty sure the escapes "" are no longer needed. I think there were for a bug we had in tooling.
@Luap99 didn't you fix something in this area.

@svdHero
Copy link
Contributor Author

svdHero commented Jan 13, 2022

@rhatdan Can I still rebase, although I already merged the main into this branch? Wouldn't I break anything if I did a rebase (=history rewrite)?

Sorry for these beginner questions. Normally I do not have to change git history and I have never done a cross-repo / cross-project PR in Github. At my company we use an on-prem Bitbucket Server.

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

Yes that is the way we prefer, one signed PR.

I am pretty sure the escapes "" are no longer needed. I think there were for a bug we had in tooling. @Luap99 didn't you fix something in this area.

Everything in backticks should not be escaped, so this LGTM

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

@svdHero You can rebase and force push without problems

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 14, 2022

Can I still rebase, although I already merged the main into this branch? Wouldn't I break anything if I did a rebase (=history rewrite)?

Yes. You merged main into your branch, you didn’t merge anything into the public main branch.

We do care about preserving history on the main branch, but permissions should prevent you from doing that, so don’t worry about breaking anything; you can always start afresh with a new checkout.

Something broadly like

git fetch --all
git checkout $yourbranch
git rebase $upstreamRemote/main # for some $upstreamRemote
git show HEAD # verify that it is now your (only) commit
git commit --amend --signoff
git push -f $yourRemote

should work, and get rid of the merge commit.

OTOH if a merge commit stays around, that’s not that big a deal either for the project. It primarily makes it more difficult for you to amend the commit with the sign-off.

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 14, 2022

Yes. You merged main into your branch, you didn’t merge anything into the public main branch.

… whereas the PR branch is intended to be short-lived, and the GitHub PR mechanisms (or at least the way we use them) don’t rely on history of that PR branch. Rebasing PR branches, for various reasons, is actually quite common.

@svdHero
Copy link
Contributor Author

svdHero commented Jan 18, 2022

@mtrmac Thanks for helping me with the git. I will add another commit incorporating your suggestions relating XDG_CONFIG_HOME.

@svdHero
Copy link
Contributor Author

svdHero commented Jan 18, 2022

@mtrmac I added the parenthetical hint and squashed the commits. The PR should be good to go.
One last question: You wrote about this documentation:

That documentation is a bit confusing, and could be improved. It is correct but it could easily be read to mean that XDG_CONFIG_HOME defaults to $HOME/.config/containers, which is not the case. Could/should I open a PR to improve that to make it less confusing?

Signed-off-by: Joerg Baeuerle <joerg.baeuerle@gmx.net>
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mtrmac mtrmac merged commit d9c79d3 into containers:main Jan 18, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Jan 18, 2022

Could/should I open a PR to improve that to make it less confusing?

I think that would be great. It’s in the Podman project, https://github.com/containers/podman/blob/main/docs/source/markdown/podman.1.md .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants