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

Update java_certificate to function on windows #723

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakauppila
Copy link
Contributor

@jakauppila jakauppila commented Mar 28, 2025

Description

Updates the install and remove actions of java_certificate to function on Windows

Adding Windows tests for just this is a bit complicated since the Java install resources don't currently support Windows.

Issues Resolved

List any existing issues this PR resolves

Check List

  • A summary of changes made is included in the CHANGELOG under ## Unreleased
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@jakauppila jakauppila requested a review from a team as a code owner March 28, 2025 05:55
@Stromweld
Copy link
Contributor

I have 2 issues with this, and neither pertain to your code or is your fault. But since this cookbook metadata doesn't indicate any support for windows I don't think we should add support to a single resource like this. It really should be fully added to support windows. The other is the lack of testing for the java cert resource. We run the test CB which utilizes the java_cert resource but then the integration tests only verify that java installed correctly. This makes it difficult to know if your change actually works or not.

@Stromweld
Copy link
Contributor

I'm not saying we shouldn't do this, just pointing that out and going to defer to others on what they may think.

@jakauppila
Copy link
Contributor Author

Understandable; I can certainly add more explicit testing for java_certificate to address that concern.

I can also see if there's a more "neutral" way for the following without explicitly performing the platform check. Might be able to just tweak the regex.

normalized_stdout = cmd.stdout
normalized_stdout = normalized_stdout.gsub(/\r\n/, "\n") if platform?('windows')
keystore_cert = normalized_stdout.match(/^[-]+BEGIN.*END(\s|\w)+[-]+$/m).to_s

Then there's less of an implicit indicator that the cookbook "supports" Windows.

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