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

fix: upgrade to aruba-2/cucumber-8 #360

Merged
merged 4 commits into from
Dec 29, 2023
Merged

fix: upgrade to aruba-2/cucumber-8 #360

merged 4 commits into from
Dec 29, 2023

Conversation

robbat2
Copy link
Contributor

@robbat2 robbat2 commented Dec 24, 2023

Update tests and code to support aruba-2/cucumber-8 as needed by Ruby 3.2. Notably, HOME is treated specially, and environment variables are handled by the test suite. HOME is nominally a relative path, so take special precautions to work with an absolute path.

Signed-off-by: Robin H. Johnson [email protected]


if public_key and public_key_env_var
warn 'both public_key and public_key_env_var specified, using public_key'
end

public_key_pem = if public_key_env_var and ENV[public_key_env_var]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the old line43-45 warning says the OPPOSITE thing to the old eval block.
Which one should be used? I chose to make the new behavior match the warning; but it's easy to flip.

Copy link
Member

Choose a reason for hiding this comment

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

Better fix the warning and keep the current behavior so that existing setup with both which produce a broken warning but work now produce a genuine warning and still work as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to have the warning match behavior but also remove the duplication of this load between the 3 key loads (encrypt public_key, decrypt public_key, decrypt private_key).

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Awesome! I added in-line replies to your comments. Thanks!

lib/hiera/backend/eyaml/subcommand.rb Outdated Show resolved Hide resolved

if public_key and public_key_env_var
warn 'both public_key and public_key_env_var specified, using public_key'
end

public_key_pem = if public_key_env_var and ENV[public_key_env_var]
Copy link
Member

Choose a reason for hiding this comment

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

Better fix the warning and keep the current behavior so that existing setup with both which produce a broken warning but work now produce a genuine warning and still work as before.

Update tests and code to support aruba-2/cucumber-8 as needed by Ruby
3.2. Notably, HOME is treated specially, and environment variables are
handled by the test suite. HOME is nominally a relative path, so take
special precautions to work with an absolute path.

Signed-off-by: Robin H. Johnson <[email protected]>
…ect code

If both public_key and public_key_env_var were set, this message
triggred:
`both public_key and public_key_env_var specified, using public_key`

However, the code actually preferred the `public_key_env_var` path.
Change the warning to reflect that.

Also change a similar line for private_key/private_key_env_var.

Signed-off-by: Robin H. Johnson <[email protected]>
Resolves: voxpupuli#360 (comment)
PKCS encrypt & decrypt had duplicate key loading logic with the same
validations. Refactor to a single function to load PEM from variable or
disk.

Signed-off-by: Robin H. Johnson <[email protected]>
@robbat2
Copy link
Contributor Author

robbat2 commented Dec 25, 2023

Those new commits should cover all of the concerns

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM

@robbat2
Copy link
Contributor Author

robbat2 commented Dec 29, 2023

@smortex can you please merge, I don't have permissions in your repo.

@tuxmea tuxmea merged commit c63f4af into voxpupuli:master Dec 29, 2023
9 checks passed
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