Skip to content
This repository was archived by the owner on Feb 16, 2025. It is now read-only.

Webd and elektrad bulk kdb meta-set #4910

Merged
merged 10 commits into from
May 1, 2023

Conversation

tmakar
Copy link
Contributor

@tmakar tmakar commented Apr 21, 2023

Basics

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add them to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy. If not, fix in this order:
    • add a line in doc/news/_preparation_next_release.md
    • reformat the code with scripts/dev/reformat-all
    • make all unit tests pass
    • fix all memleaks
    • fix the CI itself (or rebase if already fixed)
  • The PR is rebased with current master.

Checklist

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation (see Documentation Guidelines)
  • I fixed all affected decisions (see Decision Process)
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in reuse syntax

Review

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if everything is done and no further pushes are planned by you.

@tmakar tmakar requested review from atmaxinger and hannes99 April 21, 2023 22:08
@tmakar
Copy link
Contributor Author

tmakar commented Apr 21, 2023

This PR introduces:

  • Implement bulk meta key creation request (the new request supports to add multiple meta keys to a key in one request)
  • Add shell recorder test in README for this new implementation
  • Fix existing shell-recorder test to use correct key name

NOTE: Implementation is done for elektrad (GO) and for webd (NodeJS) .

@tmakar tmakar changed the title Webd bulk kdb meta-set (elektrad, webd) Webd and elektrad bulk kdb meta-set Apr 21, 2023
Copy link
Contributor

@atmaxinger atmaxinger left a comment

Choose a reason for hiding this comment

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

For me this looks okay from a pure idea standpoint. However, I can't review the code, as I've never seen a single line of Go code in my life.

It's also a bit off-putting that there are no tests, but apparently, there are no existing tests for elektrad or webd either.

@tmakar
Copy link
Contributor Author

tmakar commented Apr 21, 2023

It's also a bit off-putting that there are no tests, but apparently, there are no existing tests for elektrad or webd either.

Yes, I noticed this as well.

I now thought that it is better to put at least a shell-recorder test into the README than no test at all.

I will may in the next iterations work on adding tests, at least for the elektrad tool.

@tmakar
Copy link
Contributor Author

tmakar commented Apr 22, 2023

@atmaxinger There actually are tests for elektrad but not for webd. I was wrong. meta_handler_test.go

@tmakar tmakar requested a review from markus2330 April 22, 2023 12:32
@atmaxinger
Copy link
Contributor

please rebase to resolve merge conflict

@tmakar tmakar force-pushed the webd-bulk-kdb-set branch from 108fb5e to ce70a98 Compare April 24, 2023 14:45
@tmakar tmakar added lang/go and removed lang/go labels Apr 26, 2023
@atmaxinger
Copy link
Contributor

jenkins build libelektra please

@atmaxinger atmaxinger mentioned this pull request Apr 28, 2023
31 tasks
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

LGTM

@markus2330 markus2330 merged commit ef00c96 into ElektraInitiative:master May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants