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

Updated keymaker behaviour #454

Merged
merged 6 commits into from
Nov 11, 2021
Merged

Conversation

ZhmakAS
Copy link
Contributor

@ZhmakAS ZhmakAS commented Nov 11, 2021

Updated keymaker tool to be able to generate keys that are not using client_id with its empty definition.
client_id will be validated only for generation of keys with required client_id

Checklist

Added ability to keymaker to generate keys with empty client_id
@ZhmakAS ZhmakAS requested a review from Lagovas November 11, 2021 14:27
tests/test.py Outdated
subprocess.check_output(
['./acra-keymaker', '--keystore={}'.format(KEYSTORE_VERSION),
'--keys_output_dir={}'.format(folder),
'--client_id=''',
Copy link
Collaborator

Choose a reason for hiding this comment

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

what will print next code:

a =  '--client_id='''
print(a)

?

I suggest to rewrite this literal to '--client_id' or "--client_id=''" to be more readable what the actual value will be used. Because now we check client_id validation, not empty value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this code will produce just -client_id= which is totally fine for our scenario, but in case of simply passing --client_id it wont start because you are passing param but not specified its value:)

a =  '--client_id='''
print(a)

@ZhmakAS ZhmakAS requested a review from Lagovas November 11, 2021 15:35
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

lgtm if CI tests pass

split vault unit tests by versions
@ZhmakAS ZhmakAS merged commit 597b7b9 into master Nov 11, 2021
@Lagovas Lagovas deleted the zhars/updated_keymaker_behaviour branch February 4, 2022 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants