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

Add sanity-checks for keys to "kdb spec-mount" command #4058

Conversation

qwepoizt
Copy link
Contributor

@qwepoizt qwepoizt commented Sep 18, 2021

Improves problems described in #3983 and #3987.

(This PR should not close these issues).

I've implemented several sanity checks in the spec-mounter.

Basics

These points need to be fulfilled for every PR:

  • 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 something 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
  • The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

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 for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • 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 THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Labels

If you are already Elektra developer:

  • 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 basics are fulfilled and you also
    say that everything is ready to be merged.

@qwepoizt qwepoizt changed the title 3987 add sanity checks to spec mount Add sanity-checks for keys to "kdb spec-mount" command Sep 18, 2021
@qwepoizt
Copy link
Contributor Author

@markus2330: I've implemented several sanity checks in the spec-mounter.

I think this is a good solution for #3987 and #3983: When someone creates a specification file and then executes kdb spec-mount ... to test/use it, they will receive errors if one of the sanity checks fails!

I think I can say from experience that these checks will help a lot, especially if someone is just getting started with Elektra/specification files. They cover the mistakes I made while writing the specification for redshift.

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.

Thank you very much for this improvement! Fixes for problems you actually ran into are very valuable. Nevertheless we should leave the issues open (or create new ones) as it would be better if such inconsistencies cannot happen at all (better design of the specification).

@qwepoizt
Copy link
Contributor Author

Nevertheless we should leave the issues open.

Okay, I've edited the description of this PR so it won't auto close them!

@qwepoizt
Copy link
Contributor Author

@markus2330: Currently, a test is failing in this PR, because I have not included "int" as an allowed value for the "type" metakey, but the following test uses a specification file with "type=int".

[]
mountpoint=tests_gen_elektra_nodefault.ini
[nodefault]
type=int

Is "int" allowed?

libelektra/doc/METADATA.ini

Lines 356 to 357 in 90fb5be

[args/index]
type= int

[order]
type = int
status = implemented
usedby/plugin = hosts augeas toml

libelektra/doc/METADATA.ini

Lines 1191 to 1195 in 90fb5be

[mode]
status= deprecated
type= int
usedby/api= keyNew kdbextension.h
description= The access mode of the key.

@qwepoizt
Copy link
Contributor Author

@markus2330: Another check is also failing, namely the "Check" check:

doc/news/2018-11-18_0.8.25.md:343:0 http://coveralls.io
doc/news/2019-08-06_0.9.0.md:786:0 https://coveralls.io/github/ElektraInitiative/libelektra
doc/news/2020-05-26_0.9.2.md:274:0 https://coveralls.io/github/ElektraInitiative/libelektra
doc/news/2015-04-03_0.8.11.md:288:0 https://www.libelektra.org/ftp/elektra/pancheri2015gui.pdf
doc/news/2016-12-17_website_release.md:48:0 https://www.libelektra.org/ftp/elektra/mall2016rest.pdf
doc/news/2016-04-29_0.8.16.md:326:0 https://www.libelektra.org/ftp/elektra/berlakovich2016universal.pdf
  • Have you maybe changed something on https://www.libelektra.org/ftp/elektra/ ?
  • I also hope that coveralls.io will be back to service soon. It is in maintenance mode, responding with status 503 since Sep 17th. So far none of my PRs were blocked by this.
  • Is there an alternative to removing the coverall links from the old release notes?

@markus2330
Copy link
Contributor

markus2330 commented Sep 21, 2021

Is "int" allowed?

No, this is probably some relic from before we started to use the CORBA type system. See doc/METADATA.ini for allowed types.

It is used in a few places, e.g.:

Thank you for pointing it out, these places need to be fixed!

Have you maybe changed something on https://www.libelektra.org/ftp/elektra/ ?

@robaerd pushed a new version which contains a commit that moves publications to a subfolder with that name. The links need to be corrected. I asked him to do that but it seems like he did not have time for this yet.

I also hope that coveralls.io will be back to service soon. It is in maintenance mode, responding with status 503 since Sep 17th. So far none of my PRs were blocked by this.

If some server is down (not Jenkins) there is nothing we can do about it, so we cannot let ourselves get blocked by this. The goal is to have as much functionality as possible in Jenkins. So we can merge your PRs if only non-Jenkins jobs fail.

@qwepoizt
Copy link
Contributor Author

qwepoizt commented Sep 21, 2021

Thanks for your comment!

I've fixed the usage that made the test fail and created #4060 for a more detailed inspection of this. I've also marked it as "floss2021W", seems like a good issue to start!

I'll wait for the results of the checks.

@qwepoizt
Copy link
Contributor Author

@markus2330: This is now ready to merge.

The 4 unsuccessful checks are caused by:

@markus2330 markus2330 merged commit e84e0c5 into ElektraInitiative:master Sep 22, 2021
@markus2330
Copy link
Contributor

Thank you very much! 💖

@robaerd robaerd mentioned this pull request Sep 23, 2021
20 tasks
@mpranj mpranj added this to the 0.9.8 milestone Sep 26, 2021
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.

3 participants