Skip to content

dist: Add distrotest lib#17488

Merged
phlax merged 2 commits intoenvoyproxy:mainfrom
phlax:dist-add-distrotest
Jul 30, 2021
Merged

dist: Add distrotest lib#17488
phlax merged 2 commits intoenvoyproxy:mainfrom
phlax:dist-add-distrotest

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Jul 26, 2021

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: dist: Add distrotest lib
Additional Description:

This is a breakout from #17380 and #16899

It adds a a lib that for given arguments:

  • creates a docker image (based on a distro - eg debian/ubuntu/redhat)
  • starts a container with the image
  • runs tests against provided installables - ie debs/rpms

this doesnt include the Checker to run the tests in ci etc

theres quite a bit of code in #17380 so i figured to break the lib/tests out first

this will also allow tweaking the cli args in the checker once this has landed

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax changed the title dist: Add distrotest lib [WIP] dist: Add distrotest lib Jul 26, 2021
@phlax phlax marked this pull request as draft July 26, 2021 13:18
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jul 26, 2021

@phlax phlax force-pushed the dist-add-distrotest branch 2 times, most recently from 2aaa24f to d2e35bd Compare July 26, 2021 13:31
@phlax phlax requested a review from htuch July 26, 2021 13:37
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jul 26, 2021

cc @htuch

@phlax phlax force-pushed the dist-add-distrotest branch from d2e35bd to 7ce7ab1 Compare July 26, 2021 13:41
@phlax phlax force-pushed the dist-add-distrotest branch from 7ce7ab1 to 764623f Compare July 26, 2021 14:20
@phlax phlax changed the title [WIP] dist: Add distrotest lib dist: Add distrotest lib Jul 26, 2021
@phlax phlax marked this pull request as ready for review July 26, 2021 14:47
@phlax phlax force-pushed the dist-add-distrotest branch 2 times, most recently from a31c7b7 to 3d8a9de Compare July 26, 2021 20:20
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks really clean, just some minor comments/nits.
/wait

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you consider modeling the configuration format as proto3 (with YAML frontend) to provide a schema/documentation?

Copy link
Copy Markdown
Member Author

@phlax phlax Jul 27, 2021

Choose a reason for hiding this comment

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

the idea crossed my mind, but tbh i didnt see the immediate benefit.

I guess though, as you say, the benefit is documentation and a schema to work with

there are 2 yaml configs

this one defines the build/exec environment for the different container types - ie deb/rpm - its more complex but unlikely to change much and is not public-facing/configurable, at least with current implementation of the checker.

the other yaml schema (which comes as part of the next pr) is configurable/public-facing, but is super simple - basically just

debian_buster:
    image: debian:buster-slim
    ext: buster.changes

i would be happy following up to add protobuf schemas for these if you were ok with that

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the downside is i guess the dependency on protobuf

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Up to you, just thought it might have some win. My only strongly held opinion is that if we go with a schema, it should be protobuf, to reduce the proliferation of schema languages/tooling. Protobuf dependency isn't too bad I think, we have this in a fair bit of Python tooling already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, noted

@phlax phlax marked this pull request as draft July 27, 2021 14:18
@phlax phlax changed the title dist: Add distrotest lib [WIP] dist: Add distrotest lib Jul 27, 2021
@phlax phlax force-pushed the dist-add-distrotest branch from 3d8a9de to 4dfbc4e Compare July 28, 2021 13:09
@phlax phlax force-pushed the dist-add-distrotest branch from 4dfbc4e to b14f3ff Compare July 28, 2021 13:39
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the dist-add-distrotest branch from b14f3ff to 6ef56af Compare July 28, 2021 13:43
@phlax phlax marked this pull request as ready for review July 28, 2021 13:43
@phlax phlax changed the title [WIP] dist: Add distrotest lib dist: Add distrotest lib Jul 28, 2021
@phlax phlax requested a review from htuch July 28, 2021 13:43
`tarball` is the path to a tarball containing packages.
`keyfile` is the path to the public key used to sign the packages
`testfile` is the bash script to run inside the test containers.
`maintainer` is the expected maintainer/packager the packages were signed with.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i think its theoretically possible to derive the maintainer from the keyfile but probs i would prefer to follow up with this as it adds some complexity

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, but I think you have some changes still in flight so can approve again later. I didn't review the unit tests in depth.

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the dist-add-distrotest branch from d9656e2 to ae0e3bb Compare July 29, 2021 14:40
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@phlax phlax merged commit 537cf1a into envoyproxy:main Jul 30, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: Ryan Northey <ryan@synca.io>
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