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

feat(settings): add user settings support with defaults values and trusts #1940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

noirbizarre
Copy link
Contributor

Hello 👋🏼

This PR adds support for user settings with initial support for:

Settings are stored in <CONFIG_ROOT>/settings.yml where <CONFIG_ROOT> is the
standard configuration directory for your platform:

This location can be overridden by setting the COPIER_SETTINGS environment variable.

The settings look like:

defaults:
    user_name: "John Doe"
    user_email: [email protected]
trust:
    - https://github.com/account/template.git
    - https://github.com/org/
    - ~/templates/

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.05%. Comparing base (0f752d7) to head (211835d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1940      +/-   ##
==========================================
+ Coverage   97.90%   98.05%   +0.14%     
==========================================
  Files          51       53       +2     
  Lines        5404     5511     +107     
==========================================
+ Hits         5291     5404     +113     
+ Misses        113      107       -6     
Flag Coverage Δ
unittests 98.05% <100.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Very nice! I like the prefix matching approach for trusted repositories, it's simple and it makes sense.

copier/main.py Outdated Show resolved Hide resolved
copier/settings.py Show resolved Hide resolved
copier/settings.py Show resolved Hide resolved
docs/configuring.md Outdated Show resolved Hide resolved
docs/settings.md Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Fantastic work, @noirbizarre! 🎉 Thanks for working on and contributing these valuable features! 🙏

I have a few minor remarks. And would you mind fixing the formatting errors in the corresponding commit and drop the auto-fix commit? We'll likely want to rebase-merge this PR to keep the two feature commits, so the auto-fix commit would only add some noise to the Git history.

docs/settings.md Outdated Show resolved Hide resolved
docs/settings.md Outdated Show resolved Hide resolved
docs/settings.md Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
@noirbizarre
Copy link
Contributor Author

You're welcome ! 🙏🏼
I take the suggestion commit, and I'll rebase properly everything, that was plan from the start 😉

@noirbizarre noirbizarre force-pushed the feat/settings branch 2 times, most recently from ac3b856 to f8a881c Compare January 20, 2025 22:15
Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Just a few more comments after another review iteration. 🙂

copier/errors.py Outdated Show resolved Hide resolved
Comment on lines +50 to +61
return any(
repository.startswith(self.normalize(trusted))
if trusted.endswith("/")
else repository == self.normalize(trusted)
for trusted in self.trust
)

def normalize(self, url: str) -> str:
"""Normalize an URL using user settings."""
if url.startswith("~"): # Only expand on str to avoid messing with URLs
url = expanduser(url)
return url
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we should use copier.vcs.get_repo() to normalize trusted repository URLs to be more robust against non-canonical URLs. From the tests below, a (normalized) repository https://github.com/user/repo.git wouldn't be considered trusted despite the trust list entry https://github.com/user/repo. Won't this lead to confusion?

I realize that copier.vcs.get_repo() currently doesn't handle URLs with a trailing slash properly:

>>> get_repo("https://github.com/user/repo/")
'https://github.com/user/repo/.git'

But this could be fixed. WDYT?

Copy link
Contributor Author

@noirbizarre noirbizarre Jan 21, 2025

Choose a reason for hiding this comment

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

Yes, I noticed that when I started #1941. That's one of the reasons I separated the shortcuts into their own PR.
If this is OK for you, I would prefer handling get_repo and URL normalization in #1941 as it is its main point. It would allow having the full picture and craft a solution that handles more cases.

I think we have the opportunity to properly review/complete/fix normalization rules in #1941. (I'll properly rebase it when this one is merged so we can have a clear view)

docs/settings.md Show resolved Hide resolved
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.

3 participants