java icon indicating copy to clipboard operation
java copied to clipboard

Update `java_certificate` to function on windows

Open jakauppila opened this issue 9 months ago • 3 comments

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

  • [x] 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 avatar Mar 28 '25 05:03 jakauppila

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 avatar Mar 31 '25 18:03 Stromweld

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

Stromweld avatar Mar 31 '25 18:03 Stromweld

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.

jakauppila avatar Mar 31 '25 19:03 jakauppila