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

FMF metadata support to colin #141

Closed
wants to merge 15 commits into from
Closed

Conversation

jscotka
Copy link
Collaborator

@jscotka jscotka commented May 31, 2018

FMF Metadata works well

FMF metadata support in colin. There are two different parts

  • test objects can read FMF metadata from files on demand
  • scheduler
    • dynamic nosetests generator
    • FMF rulesets support
    • filtering abilities

TODOs:

Colin output:

$ python -m colin.cli.colin list-checks

maintainer_label
   -> Label 'maintainer' has to be specified.
   -> The name and email of the maintainer (usually the submitter).
   -> https://fedoraproject.org/wiki/Container:Guidelines#LABELS
   -> maintainer, label, required

from_tag_not_latest
   -> In FROM, tag has to be specified and not 'latest'.
   -> Using the 'latest' tag may cause unpredictable builds.It is recommended that a specific tag is used in the FROM.
   -> https://fedoraproject.org/wiki/Container:Guidelines#FROM
   -> from, dockerfile, baseimage, latest, required

maintainer_deprecated
   -> Dockerfile instruction `MAINTAINER` is deprecated.
   -> Replace with label 'maintainer'.
   -> https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
   -> maintainer, dockerfile, deprecated, required

python -m colin.cli.colin check tests/data/Dockerfile

PASS:Label 'maintainer' has to be specified.
PASS:In FROM, tag has to be specified and not 'latest'.
PASS:Dockerfile instruction `MAINTAINER` is deprecated.

PASS:3 

FMF output

fmf show --path colin/checks

/best_practices/cmd_or_entrypoint
class: CmdOrEntrypointCheck
description: An ENTRYPOINT allows you to configure a container that will run as an executable. The main purpose of a CMD is to provide defaults for an executing container.
message: Cmd or Entrypoint has to be specified
reference_url: https://fedoraproject.org/wiki/Container:Guidelines#CMD.2FENTRYPOINT_2
tags: cmd and entrypoint
test: best_practices.py

/best_practices/help_file_or_readme
all_must_be_present: False
class: HelpFileOrReadmeCheck
description: Just like traditional packages, containers need some 'man page' information about how they are to be used, configured, and integrated into a larger stack.
files: /help.1 and /README.md
message: The 'helpfile' has to be provided.
reference_url: https://fedoraproject.org/wiki/Container:Guidelines#Help_File
tags: filesystem, helpfile and man
test: best_practices.py

/best_practices/no_root
class: NoRootCheck
description: It can be insecure to run service as root.
message: Service should not run as root by default.
reference_url: ?????
tags: root and user
test: best_practices.py

/dockerfile/from_tag_not_latest
class: FromTagNotLatestCheck
description: Using the 'latest' tag may cause unpredictable builds.It is recommended that a specific tag is used in the FROM.
message: In FROM, tag has to be specified and not 'latest'.
reference_url: https://fedoraproject.org/wiki/Container:Guidelines#FROM
tags: from, dockerfile, baseimage and latest
test: dockerfile.py

/dockerfile/maintainer_deprecated
class: MaintainerDeprecatedCheck
description: Replace with label 'maintainer'.
instruction: MAINTAINER
max_count: 0
message: Dockerfile instruction `MAINTAINER` is deprecated.
reference_url: https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
tags: maintainer, dockerfile and deprecated
test: dockerfile.py

/labels/maintainer_label
class: MaintainerLabelCheck
description: The name and email of the maintainer (usually the submitter).
labels: maintainer
message: Label 'maintainer' has to be specified.
reference_url: https://fedoraproject.org/wiki/Container:Guidelines#LABELS
required: True
tags: maintainer and label
test: labels.py
value_regex: None
when I want to be supercool and have backward compilant output:

use format abilites of fmf:
fmf show --path colin/checks --format "{}\n -> {}\n -> {}\n -> {}\n -> {}\n\n" --value name --value "data['description']" --value "data['message']" --value "data['reference_url']" --value "data['tags']"

/best_practices/cmd_or_entrypoint
    -> An ENTRYPOINT allows you to configure a container that will run as an executable. The main purpose of a CMD is to provide defaults for an executing container.
    -> Cmd or Entrypoint has to be specified
    -> https://fedoraproject.org/wiki/Container:Guidelines#CMD.2FENTRYPOINT_2
    -> ['cmd', 'entrypoint']

/best_practices/help_file_or_readme
    -> Just like traditional packages, containers need some 'man page' information about how they are to be used, configured, and integrated into a larger stack.
    -> The 'helpfile' has to be provided.
    -> https://fedoraproject.org/wiki/Container:Guidelines#Help_File
    -> ['filesystem', 'helpfile', 'man']

/best_practices/no_root
    -> It can be insecure to run service as root.
    -> Service should not run as root by default.
    -> ?????
    -> ['root', 'user']

/dockerfile/from_tag_not_latest
    -> Using the 'latest' tag may cause unpredictable builds.It is recommended that a specific tag is used in the FROM.
    -> In FROM, tag has to be specified and not 'latest'.
    -> https://fedoraproject.org/wiki/Container:Guidelines#FROM
    -> ['from', 'dockerfile', 'baseimage', 'latest']

/dockerfile/maintainer_deprecated
    -> Replace with label 'maintainer'.
    -> Dockerfile instruction `MAINTAINER` is deprecated.
    -> https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
    -> ['maintainer', 'dockerfile', 'deprecated']

/labels/maintainer_label
    -> The name and email of the maintainer (usually the submitter).
    -> Label 'maintainer' has to be specified.
    -> https://fedoraproject.org/wiki/Container:Guidelines#LABELS
    -> ['maintainer', 'label']

Nosetests output - direct

TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py

test (fmf_scheduler.from_tag_not_latest) ... ok
test (fmf_scheduler.maintainer_deprecated) ... ok
test (fmf_scheduler.maintainer_label) ... ok

----------------------------------------------------------------------
Ran 3 tests in 0.003s

OK

using rulesets

RULESETPATH=rulesets/ TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py
test (fmf_scheduler.check@from_tag_not_latest) ... ok
test (fmf_scheduler.check@maintainer_deprecated) ... ok
test (fmf_scheduler.check@maintainer_label) ... ok

advanced filtering

possible to apply them to rulesets or directy to testcases
FILTERS=tags:dockerfile TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py

test (fmf_scheduler.from_tag_not_latest) ... ok
test (fmf_scheduler.maintainer_deprecated) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.001s

OK

$ NAMES="from_tag_not_latest;maintainer_label" TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py

fmf_scheduler.py
test (fmf_scheduler.from_tag_not_latest) ... ok
test (fmf_scheduler.maintainer_label) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.002s

OK
rulesets filters

$ NAMES=default RULESETPATH=rulesets/ TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py

test (fmf_scheduler.check@from_tag_not_latest) ... ok
test (fmf_scheduler.check@maintainer_deprecated) ... ok
test (fmf_scheduler.check@maintainer_label) ... ok

----------------------------------------------------------------------
Ran 3 tests in 0.003s

OK

$ NAMES=fedora RULESETPATH=rulesets/ TARGET=tests/data/Dockerfile nosetests -d -v fmf_scheduler.py
fedora ruleset is not given there, so that it runs 0 tests

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK

@jscotka jscotka force-pushed the fmf_metadata branch 2 times, most recently from a5c8249 to f8c4ef7 Compare May 31, 2018 14:27
@user-cont user-cont deleted a comment May 31, 2018
@user-cont user-cont deleted a comment May 31, 2018
@user-cont user-cont deleted a comment May 31, 2018
@user-cont user-cont deleted a comment May 31, 2018
@user-cont user-cont deleted a comment May 31, 2018
@user-cont user-cont deleted a comment May 31, 2018
Copy link
Member

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

It would be for the best to discuss this in person.

@@ -0,0 +1,19 @@
description: "asdasdsfdasfsda f"
Copy link
Member

Choose a reason for hiding this comment

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

what format is this? is it yaml?

def _get_fmf_metadata(self):
output = {}
classfile = inspect.getfile(self.__class__)
fmf_tree = fmf.Tree(os.path.dirname(classfile))
Copy link
Member

Choose a reason for hiding this comment

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

does this mean that we would have to store the metadata alongside the .py file?

setattr(self, k, metadata_dict[k])

def __init__(self, message=None, description=None, reference_url=None, tags=None):
if message or description or reference_url or tags:
Copy link
Member

Choose a reason for hiding this comment

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

better logic could be to load the metadata and just enable overriding them

@user-cont user-cont deleted a comment Jun 1, 2018
@user-cont user-cont deleted a comment from TomasTomecek Jun 1, 2018
@user-cont user-cont deleted a comment Jun 1, 2018
@jscotka jscotka force-pushed the fmf_metadata branch 2 times, most recently from af8f0e9 to caf053c Compare June 4, 2018 08:46
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jun 4, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@user-cont user-cont deleted a comment Jul 16, 2018
@@ -0,0 +1,188 @@
#!/usr/bin/python3
Copy link
Member

Choose a reason for hiding this comment

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

Since this script actually proposes a new runtime, I would personally prefer to have it proposed in a different PR. I actually dislike that it's not integrated into colin's CLI interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, this is my suggested solution, split this PR, to 2 or 3 PRs, and last of them will be this nosetests generator.

But on other hand, without this, it is harder to show benefits eg:

  • write tests just in metadata without empty python classes
  • using nosetests scheduled tests
  • filtering and selecting or test items
  • using FMF rulesets

And this PR will just split metadata outside python classes to FMF files.
But I agree and with this knowledge, that these PRs are closely connected I can split it.

reference_url="https://fedoraproject.org/wiki/Container:Guidelines#FROM",
tags=["from", "dockerfile", "baseimage", "latest"])
class FromTagNotLatestCheck(FMFAbstractCheck, DockerfileAbstractCheck):
name, metadata = FMFAbstractCheck.get_metadata("from_tag_not_latest")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether there is a nicer way of doing this. Calling functions on class' attributes outside of methods seems sketchy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I had also troubles with this, but actually there is probably not better way how to do it without bigger changes in colin. Because current colin implementation does static inspection of classes, to find checks based on class attribute name. If you have some idea I'll be happy to change it to better way

@jscotka
Copy link
Collaborator Author

jscotka commented Jul 17, 2018

This PR can be closed, PR splitted to more PRs:
#159
#160
#161

@jscotka jscotka closed this Jul 17, 2018
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