Skip to content

Conversation

@arvindth
Copy link
Member

@arvindth arvindth commented Aug 14, 2019

Changes:

  • Update to use the confluent fork of the underlying go library for property file parsing & writing
  • Remove previous workaround to add/remove a final dummy key/value pair to preserve trailing comments in the input property files. This is now handled natively by the updated library.
  • Update the call to read/write property files to use the new preserveFormatting option.

Note:
Upstream PR - magiconair/properties#38

  • If it gets accepted upstream, cli should be reverted to point back to the upstream library and the confluentinc/properties fork should be deleted.

@arvindth arvindth requested a review from a team as a code owner August 14, 2019 20:26
@arvindth
Copy link
Member Author

@ybyzek, I've tested this change against some variations of properties files based on your original description in the ticket. However, as the original reporter I would request that you try out this branch on some properties files to ensure that it meets the ask from the ticket.

@arvindth arvindth requested a review from ybyzek August 14, 2019 20:32
@ybyzek
Copy link

ybyzek commented Aug 14, 2019

@arvindth awesome work! However, I'm very slammed and won't have time to validate this. Please validate with QA, PM, or others...please don't block on me. Thanks!

@arvindth
Copy link
Member Author

arvindth commented Aug 14, 2019

@confluentinc/cli, while reviewing this, note that this depends on an update to the underlying magiconair/properties library, which I've done on a confluentinc fork (confluentinc/properties#1). Please let me know if you think the fork's changes should also go through a PR review.

@DABH
Copy link
Contributor

DABH commented Aug 19, 2019

@arvindth no need for us reviewing that lib, that is between you and @magiconair : )

Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

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

LGTM

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