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

refactor: clean up secret questions management #1209

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

sisp
Copy link
Member

@sisp sisp commented Jun 20, 2023

I've cleaned up the internal bookkeeping logic for secret questions. The new structure will simplify raising an error when a secret question has no default value, which we discussed in #1177 (comment).

There are two ways to declare a question as secret:

  1. By setting secret: true in the config of a question.
  2. By adding the variable name of a question to the _secret_questions list. This option is needed when a question that uses the <name>: <default value> syntax shall be secret.

Internally, Copier uses the a secret questions list to omit answers to secret questions from the answers file. This list contains the variable names of all secret questions independent of how they are declared as secret, so questions with secret: true are added to this list internally.

Before, adding questions with the secret: true setting to the _secret_questions list took place both in template.py:filter_config() and in template.py:Template.secret_questions which was redundant and also a bit error-prone because there was no clear separation of concerns where this information shall be synced.

I've decided to let template.py:filter_config() only handle separating config and questions data as its docstring suggests, and sync the secret information in template.py:Template.secret_questions (as it has been done already, so no change) and template.py:Template.questions_data.

In addition, I've fixed some minor oddities:

  • A literal dict {} was used as a default value for getting the _secret_questions list, which should be a list.
  • The empty config data was {"secret_questions": set()} which makes not much sense (or is at least unnecessary).

Those changes are purely internal though.

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #1209 (525f187) into master (325ace4) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1209      +/-   ##
==========================================
- Coverage   96.75%   96.52%   -0.23%     
==========================================
  Files          47       47              
  Lines        3942     3942              
==========================================
- Hits         3814     3805       -9     
- Misses        128      137       +9     
Flag Coverage Δ
unittests 96.52% <100.00%> (-0.23%) ⬇️

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

Impacted Files Coverage Δ
copier/template.py 96.31% <100.00%> (ø)
tests/test_config.py 99.08% <100.00%> (ø)

... and 4 files with indirect coverage changes

@yajo yajo enabled auto-merge (squash) June 29, 2023 15:59
@sisp
Copy link
Member Author

sisp commented Jun 29, 2023

Just FYI, @yajo: Auto-merge is a bit useless now because a second approval is needed. And I can't approve my own PR. So you or @pawamoy still need to take action.

@yajo
Copy link
Member

yajo commented Jul 8, 2023

Thanks. I downgraded that to 1 required approval.

@sisp
Copy link
Member Author

sisp commented Jul 8, 2023

@yajo The CI check CI / flake-check (macos-latest) keeps failing with this error:

  Could not set environment: 150: Operation not permitted while System Integrity Protection is engaged
  Error: Process completed with exit code 150.

Any idea what's wrong?

@sisp
Copy link
Member Author

sisp commented Jul 10, 2023

I think it has been fixed in install-nix-action v22.

@sisp sisp force-pushed the refactor/secret-questions branch from 708eac9 to 525f187 Compare July 10, 2023 18:11
@sisp
Copy link
Member Author

sisp commented Jul 10, 2023

I just noticed that install-nix-action had already been bumped to v22 by #1208, so I just rebased this PR.

@yajo yajo merged commit 7fbf287 into copier-org:master Jul 10, 2023
@sisp sisp deleted the refactor/secret-questions branch July 10, 2023 18:54
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.

2 participants