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

If user full_name and email not already set in the user config, automatically retrieve them from git config #1880

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vemonet
Copy link
Contributor

@vemonet vemonet commented Jul 4, 2023

The proposal

Providing the user name and email are really common request for a lot of cookiecutter templates. Currently it is possible to create a file ~/.cookiecutterrc to define it, but it requires additional work to do for each developer when they install cookiecutter. And cookiecutter's default are good, so from my experience most users never need to look into the extra work of setting up the user config file

Ideally cookiecutter should try to automatically find the name/email of the user when it is not explicitly set in the ~/.cookiecutterrc

A lot of developers are already storing their name and email when they setup git (and I guess most developers use git). It would be great if cookiecutter could get the name and email directly from git, as it will set properly for a lot of users

The implementation

I added a function add_user_config_from_git(config) that automatically adds the user full_name and email to the user config if they are not set already.

I picked full_name and email because they are mentioned in the docs when talking about the ~/.cookiecutterrc

The function tries to retrieve the info from the env variable GIT_AUTHOR_NAME first, then from the command git config --get user.name (and same for the email)

I took inspiration from how the hatch python packaging tool is doing it

Alternative implementation

A potential change we could make is to materialize the name/email extracted from git in the ~/.cookiecutterrc (and create the file if it does not exist), this way might be a bit clearer for the user, and let them more easily change the default name/email set if it is not the right one.

I did not wanted to alter too much how cookiecutter currently handles user config though, automatically creating the .cookiecutterrc might have unexpected side effect on the current approach used by cookiecutter to handle user config.

Conclusion

For this setup to really help the users ideally cookiecutter should encourage its users to use specific properties label for common metadata (e.g. full_name, email) so that those fields will be automatically filled for most users

I was not sure if I should have created an issue to discuss it first, but the issue template seems to be more targeted towards reporting bugs, so I sent a PR directly

Currently all tests passes, but we miss ~6 lines on the coverage. I will add the necessary tests to cover those lines if there is interest in merging this feature

I tried it, and it works as expected whenever a user config is set or not

Let me know if this is a feature that you would be interested to merge in cookiecutter, and if there is any improvements you want to bring to the implementation

…e provided by the user in the user config, it will try to get the user name (full_name) and email from the git env variable or Vincent Emonet. This enable to have the full_name and email of the user automatically set to the right value (most of the time developers set their git config)
@ericof
Copy link
Member

ericof commented Jul 4, 2023

Hello @vemonet!
Thanks for the initiative, but I would ask you also to open an issue and add tests to your change

@vemonet
Copy link
Contributor Author

vemonet commented Jul 4, 2023

Hi @ericof , ok I opened the issue, I will add the test if there is interest for adding the feature

I don't want to spend 20 minutes of my time struggling to get this last 1% of coverage to be told the feature is not interesting for the community :p

@ericof
Copy link
Member

ericof commented Jul 5, 2023

It think it is interesting, but we need to make sure it works for everyone;-)

@vemonet
Copy link
Contributor Author

vemonet commented Jul 5, 2023

@ericof I just checked the linting results more in details and found out bandit is not happy about us using subprocess :

>> Issue: [B404:blacklist] Consider possible security implications associated with the subprocess module.
   Severity: Low   Confidence: High
   CWE: CWE-78 (https://cwe.mitre.org/data/definitions/78.html)
   More Info: https://bandit.readthedocs.io/en/0.0.0/blacklists/blacklist_imports.html#b404-import-subprocess
   Location: cookiecutter/config.py:87:12
86	        if full_name is None:
87	            import subprocess
>> Issue: [B607:start_process_with_partial_path] Starting a process with a partial executable path
   Severity: Low   Confidence: High
   CWE: CWE-78 (https://cwe.mitre.org/data/definitions/78.html)
   More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b607_start_process_with_partial_path.html
   Location: cookiecutter/config.py:90:28
90	                full_name = subprocess.check_output(
91	                    ['git', 'config', '--get', 'user.name'], text=True
92	                ).strip()
>> Issue: [B603:subprocess_without_shell_equals_true] subprocess call - check for execution of untrusted input.
   Severity: Low   Confidence: High
   CWE: CWE-78 (https://cwe.mitre.org/data/definitions/78.html)
   More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b603_subprocess_without_shell_equals_true.html
   Location: cookiecutter/config.py:104:24
104	                email = subprocess.check_output(
105	                    ['git', 'config', '--get', 'user.email'], text=True
106	                ).strip()

In my opinion we can ignore those 3 issues, as they have a low severity, and if you read them the security issues only comes if the command you run in the subprocess can be changed by the user. In our case the command is hardcoded so I don't really see how it could be a vulnerability

How do we do to get the tests to pass? The only way I see would be to add an ignore for B404, B603 and B607 in the bandit config file (or some inline ignore if bandit allows it), any other idea?

@vemonet
Copy link
Contributor Author

vemonet commented Jul 6, 2023

@ericof I added the tests to reach 100% coverage

All we need now is to figure out what we do with bandit complaining about using subprocess, do we add some ignore rule for bandit?

@lsorber
Copy link

lsorber commented Feb 28, 2024

Is there still an intention to proceed with this PR?

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.

None yet

3 participants