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

Documentation/rest api web #2770

Merged

Conversation

Projects
None yet
4 participants
@dmoisej
Copy link
Contributor

commented Jun 10, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

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 the build server is happy and also you
    say that everything is ready to be merged.

@markus2330 @sanssecours please review my pull request

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@markus2330 @sanssecours the pull request is related to the #2746

It is just another copy as I was forced to refork the repository. The reason for that was, that I mistakenly added commits to the master, which are not merged into this (main) repository, so I get those changes on every pull request.

Deleting commits with the git rebases didn't help me. Additionally I had some problems with jenkins tool, which checks, if the commit can be merged or not.

@dmoisej dmoisej referenced this pull request Jun 10, 2019

Closed

Add example how to test rest api on localhost #2746

2 of 9 tasks complete
@markus2330
Copy link
Contributor

left a comment

Thank you for the improved tutorial

- `cd libelektra/src/tools/web`
- `cd elektrad`
- `npm install`
- `npm start` (replaces `kdb run-elektrad`)

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

What means "replaces" here? From source you use npm start, if installed kdb run-elektrad. It seems to be that this instructions duplicate the instructions above or web/elektrad?

This comment has been minimized.

Copy link
@dmoisej

dmoisej Jun 10, 2019

Author Contributor

Actually, I just copy pasted what is written in https://www.libelektra.org/tools/web in the section "Running from source".

It means, that all 4 commands:

  • cd libelektra/src/tools/web
  • cd elektrad
  • npm install
  • npm start

replace kdb run-elektrad

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

Actually, I just copy pasted

Please don't do this.

https://www.libelektra.org/tools/web is generated from this document.

- `npm start` (replaces `kdb run-elektrad`)

- by installing elektrad tool together with Elektra and run it
- please see the documentation [here](https://www.libelektra.org/tools/web) on how to install and run the elektra-web tool

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

Is this self-reference on purpose?

This comment has been minimized.

Copy link
@dmoisej

dmoisej Jun 10, 2019

Author Contributor

oh sorry, I didn't notice that, I will improve this

Let's create the new key-value pair `user/test`:

```sh
kdb set user/test 5

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

How to set via REST API?

"value": "5",
"meta": ""
}
```

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

Please refer to the API docu for this.

This comment has been minimized.

Copy link
@dmoisej

dmoisej Jun 10, 2019

Author Contributor

Will improve

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 12, 2019

Contributor

Not yet done?

This comment has been minimized.

Copy link
@dmoisej

dmoisej Jun 12, 2019

Author Contributor

@markus2330 it is done. In the file I showed, how the user can set a value via curl

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@markus2330, I updated the guide, please check

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

The installation procedure is still copy&pasted?

@markus2330 markus2330 requested a review from kodebach Jun 10, 2019

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@markus2330 , no, the installation of the web-elektrad tool is not copy pasted. Right now, I just added the note, "please see the section Building with elektra-web Tool".

The only thing, which is copy pasted, is four commands to start the server manually:
cd libelektra/src/tools/web
cd elektrad
npm install
npm start

But everything others is written newly.

@sanssecours

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

It is just another copy as I was forced to refork the repository. The reason for that was, that I mistakenly added commits to the master, which are not merged into this (main) repository, so I get those changes on every pull request.

You can always just create another branch based on the master branch (or any other branch) of this repository. Usually you do not need to

  • create a new fork of a repository you already cloned, or
  • create a new PR (just rebase and force-push your changes)

. Anyway, I would recommend you read the book “Git Pro” to learn a little bit more about the versioning tool. I also recommend you use a GUI, which usually simplifies working with git quite a lot. If you have a Mac (or use Windows), then I recommend you use Fork. Tower is even better in my experience, but is quite expensive and uses a really user-unfriendly licensing model.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@sanssecours, thank you for your recommendation. I didn't know, that I could create the new branch from master of the main repository as I haven't worked with forked repositories before. But I will take it into account right now. Sorry for inconveniences.

@sanssecours
Copy link
Member

left a comment

Thank you for incorporating the changes I suggested. Looks like the formatting check still fails. Can you please follow the steps suggested by the build server, or alternatively, format the ReadMe with prettier, as described here?

Show resolved Hide resolved doc/news/_preparation_next_release.md
Show resolved Hide resolved doc/news/_preparation_next_release.md
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md Outdated
Show resolved Hide resolved src/tools/web/README.md
Show resolved Hide resolved src/tools/web/README.md
@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@sanssecours I will try to fix code formatting later today.

@kodebach
Copy link
Contributor

left a comment

Apart from the stuff that @markus2330 already requested, this looks good to me.

@dmoisej dmoisej force-pushed the dmoisej:documentation/rest-api-web branch from f1f2f21 to fa1782b Jun 11, 2019

Moiseiuk added some commits Jun 12, 2019

Moiseiuk
Moiseiuk
@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@sanssecours , @markus2330 I was forced to change the output format from json to sh because in the doc/CODING.md, on the line 376, it is specified:

  • README.md and tutorials should be written exclusively with shell recorder syntax so that we know that the code in the tutorial produces output as expected

I checked then README.md with prettier tool after the changes and everything works well right now.

Additionally to this, I tried to reformat the README.md file together with scripts/reformat-markdown script from the libelektra project root.
After running:

sh scripts/reformat-markdown src/tools/web/README.md

I've got the error:

scripts/reformat-markdown: Zeile 7: $'\r': Kommando nicht gefunden.

@sanssecours

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@sanssecours , @markus2330 I was forced to change the output format from json to sh because in the doc/CODING.md, on the line 376, it is specified:

  • README.md and tutorials should be written exclusively with shell recorder syntax so that we know that the code in the tutorial produces output as expected

Please do not use incorrect language specifiers. The code block in question contains JSON code, not code for the Bourne shell (aka sh). The language specifier should reflect this.

You do not need to write this tutorial using (Markdown) Shell Recorder syntax, even if the sentence you quoted might suggest otherwise.

After running:

sh scripts/reformat-markdown src/tools/web/README.md

I've got the error:

scripts/reformat-markdown: Zeile 7: $'\r': Kommando nicht gefunden.

Interesting. Did you maybe change the line endings of reformat-markdown from Unix (\n) to DOS line endings (\r\n)?

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@sanssecours ,

Please do not use incorrect language specifiers. The code block in question contains JSON code, not code for the Bourne shell (aka sh). The language specifier should reflect this.

You do not need to write this tutorial using (Markdown) Shell Recorder syntax, even if the sentence you quoted might suggest otherwise.

Okay but if the build is failed because I specify any other language, then sh, so what should I do right now? Should I change something? If yes, than what exactly should I change and on what? Or will my pull request will be merged?

Interesting. Did you maybe change the line endings of reformat-markdown from Unix (\n) to DOS line endings (\r\n)?

I tried it quickly form my other laptop, where I have windows installed. In the evening I will try to execute this script on other laptop, where I have linux installed.

@sanssecours

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

If yes, than what exactly should I change and on what?

I just pushed a commit that contains my suggested changes.

Or will my pull request will be merged?

I think as soon as Markus and the build server are happy with the status of the PR, Markus will merge your updates.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@sanssecours thank you very much for help.

@markus2330 markus2330 merged commit ea418f1 into ElektraInitiative:master Jun 12, 2019

9 of 12 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.