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

Turn IgnoredMatch into a dataclass #252

Closed
agateau-gg opened this issue May 31, 2022 · 8 comments · Fixed by #258
Closed

Turn IgnoredMatch into a dataclass #252

agateau-gg opened this issue May 31, 2022 · 8 comments · Fixed by #258
Labels
good first issue Good for newcomers type:techdebt Fix non-optimal code

Comments

@agateau-gg
Copy link
Collaborator

agateau-gg commented May 31, 2022

IgnoredMatch is currently defined as a Dict[str, str] with two entries: name and match.

It should be turned into a dataclass to improve type-safety and make code more expressive. The Cache code must be updated as well, since it also uses the IgnoredMatch dict.

@agateau-gg agateau-gg added good first issue Good for newcomers type:techdebt Fix non-optimal code labels May 31, 2022
@agateau-gg agateau-gg changed the title tech debt: turn IgnoredMatch into a dataclass Turn IgnoredMatch into a dataclass May 31, 2022
@karo-fox
Copy link
Contributor

karo-fox commented Jun 1, 2022

Can I help with this one?

@agateau-gg
Copy link
Collaborator Author

Can I help with this one?

Sure, you are more than welcome to work on this! It's better if you wait until #247 is merged though: turning IgnoredMatch into a dataclass is the continuation of the work started with the rework of the configuration file. It should be merged today hopefully 🤞🏻.

@karo-fox
Copy link
Contributor

karo-fox commented Jun 3, 2022

Can I start working on it now?

@agateau-gg
Copy link
Collaborator Author

Yes, you can 👍🏻

@karo-fox
Copy link
Contributor

karo-fox commented Jun 3, 2022

Should I create branch from main?

@agateau-gg
Copy link
Collaborator Author

No, it's better if you start from the iac branch: the main branch does not contain the new configuration code yet.

@karo-fox
Copy link
Contributor

karo-fox commented Jun 5, 2022

Should IgnoredMatch support loading from config file with list of secrets? Like this:

matches-ignore:
  - 530e5a4a7ea00814db8845dd0cae5efaa4b974a3ce1c76d0384ba715248a5dc1
  - MY_TEST_CREDENTIAL

@agateau-gg
Copy link
Collaborator Author

No, the v2 configuration file format only supports the syntax where a match is declared as an object with name and match keys. So this should be supported:

# correct
secret:
  ignored-matches:
  - name: some secret
    match: 530e5a4a7ea00814db8845dd0cae5efaa4b974a3ce1c76d0384ba715248a5dc1
  - name: another secret
    match: 559583106f3b760810fcb41d7cb99da6e7d118912cdeca19a57358c4b6a6a627

But this should not:

# incorrect
secret:
  ignored-matches:
  - 530e5a4a7ea00814db8845dd0cae5efaa4b974a3ce1c76d0384ba715248a5dc1
  - 559583106f3b760810fcb41d7cb99da6e7d118912cdeca19a57358c4b6a6a627

@agateau-gg agateau-gg linked a pull request Jun 8, 2022 that will close this issue
agateau-gg added a commit that referenced this issue Jun 8, 2022
…aclass-#252

refactor: turn IgnoredMatch into a dataclass (#252)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type:techdebt Fix non-optimal code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants