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

error: Enabled v and d as command line options for all commands #2743 #2755

Merged

Conversation

Projects
None yet
2 participants
@Piankero
Copy link
Contributor

commented Jun 6, 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.

@Piankero Piankero requested a review from markus2330 Jun 6, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I tested it with kdb export -vd ... as written in #2743 and it works again just fine.
I removed all short options like you suggested.

Unfortunately the error triggering you proposed in #2704 (comment) does not work.

@markus2330
Copy link
Contributor

left a comment

Looks good. Please add some tests and update the docu (for all the files in doc/help which now got new arguments).

@Piankero Piankero force-pushed the Piankero:cmd-option-for-errormsg-fix branch from ae85c5c to 4e90e38 Jun 13, 2019

Piankero added some commits Jun 13, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Looks good. Please add some tests and update the docu (for all the files in doc/help which now got new arguments).

Docu is updated. I tried to patch the error plugin but without success to trigger an error for the export.

I added an extra if as discussed in the meeting:

		meta = keyGetMeta (cur, "trigger/error/delayed");
		if (meta)
		{
			keySetMeta(cur, "trigger/error", keyString (meta));
		}

It also works and locks the whole configuration because any kdb set command triggers an error on the respective mountpoint. But no triggers for get or any other command.

I fear that the only secure way to trigger errors is to manipulate on the file basis as done in my test

@markus2330 markus2330 merged commit 6da49cf into ElektraInitiative:master Jun 15, 2019

11 of 13 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
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
coverage/coveralls Coverage increased (+57.8%) to 58.032%
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 15, 2019

Docu is updated.

Thank you very much!

I added an extra if as discussed in the meeting:

Please create a separate PR for this. (Error plugin and tests improvements)

But no triggers for get or any other command.

Because currently elektraErrorGet does not check for a trigger.

I fear that the only secure way to trigger errors is to manipulate on the file basis as done in my test

Your test is not bad per-se (we can keep it) but it relies on internals of the dump plugin (which has nothing to do with your test). This should be avoided if possible, or at least also have alternative tests so that we can drop the test once it stopped working.

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.