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

feat: use helm as a libary instead of copying code #77

Merged
merged 5 commits into from
May 17, 2024

Conversation

qvalentin
Copy link
Collaborator

This change would include the official helm code as a dependency instead of copying only the code used for linting etc. to this project.

I would really like to get your input @mrjosh on such a design decision. Other people are also welcome to the discussion.

Advantages:

  • Less code in this repo that is hard to maintain
  • New lints would be added
  • We could include more features interacting with helm directly in the future

Disadvantages:

  • Larger binary size (went from 49M to 74M) without compression and to 29M using upx
  • Possible breaking changes when updating (adding enough tests should avoid this)

@qvalentin
Copy link
Collaborator Author

This would revert #14

@qvalentin
Copy link
Collaborator Author

Using helm as a library does not seem uncommon, quite a lot of projects do this: https://github.com/helm/helm/network/dependents?package_id=UGFja2FnZS0yMjY3Mjg4NjYw

@mrjosh
Copy link
Owner

mrjosh commented Apr 27, 2024

Hi @qvalentin thanks for the info regarding using helm-core as library.

By leveraging the Helm code directly, we inherently reduce the overhead associated with maintaining a separate, potentially divergent codebase. This shift not only streamlines our development process but also ensures that we benefit from the latest improvements and features added to Helm, keeping our project up-to-date and robust.

Regarding the concerns about the increased binary size and potential breaking changes, these are valid points. However, the reduction in binary size with compression to 29M is quite efficient, and by prioritizing thorough testing, we can mitigate the risks of breaking changes effectively. This proactive approach in testing will safeguard our project's stability and reliability.

@qvalentin
Copy link
Collaborator Author

Thanks for your opinion, I will take this as a approve for the proposed changes and will make it ready to be merged soon.

@qvalentin qvalentin force-pushed the feature/helm-as-libary branch from 267c880 to d7ebaa0 Compare May 9, 2024 14:35
@qvalentin qvalentin force-pushed the feature/helm-as-libary branch 3 times, most recently from 5a1d672 to d46aaab Compare May 11, 2024 12:54
internal/util/values.go Outdated Show resolved Hide resolved
@qvalentin qvalentin force-pushed the feature/helm-as-libary branch 2 times, most recently from 48e1f3c to 736cc78 Compare May 17, 2024 14:59
@qvalentin qvalentin force-pushed the feature/helm-as-libary branch from 736cc78 to fa2a239 Compare May 17, 2024 15:01
@qvalentin qvalentin merged commit c50176c into master May 17, 2024
4 checks passed
Krismix1 added a commit to Krismix1/helm-ls that referenced this pull request May 26, 2024
* fork-master: (35 commits)
  Build only linux amd64
  Support list of glob patterns
  feat: support values lookup for range on mapping (mrjosh#86)
  feat(symbol-table): read in template context with variables (mrjosh#85)
  fix: prevent panic on empty or unsupported template context (mrjosh#83)
  feat: use helm as a libary instead of copying code (mrjosh#77)
  feat: completion for unfinished_selector_expression
  fix: update ts grammar
  feat(completion): tests and refactoring
  feat(hover): template context hover usecase
  feat: references for templateContext
  feat: add references
  feat(reference): symbol-tables provides definition references
  feat(symboltable): add symboltable
  feat(docs): add coc config and reword readme
  build(deps): bump golang.org/x/net from 0.19.0 to 0.23.0
  fix(handler): remove all unimplemented panics (mrjosh#73)
  fix(hover): format numbers correctly
  fix(hover): format list as yaml
  fix(go-to-definition): support range values
  ...
@qvalentin qvalentin deleted the feature/helm-as-libary branch July 21, 2024 15:21
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