Add lib to preflight #809

sebrandon1 · 2022-10-20T22:00:42Z

Work in progress!

Related to:

This PR provides some changes needed to implement the lib. (Name is subject to change 😄)

I want to go through some of the files to describe my notable changes:

certification/runtime/config.go
In this file, I added NewManualContainerConfig and NewManualOperatorConfig for being able to create config objects geared towards their individual purposes.
cmd/check_container.go and cmd/check_operator.go
These files have had their "NewOperator/Container" funcs moved to the lib.
fakes.go and types.go
Moved to the lib folder, because it contains funcs like NewFakePyxisClientNoop and NewNoopSubmitter.
The NoopSubmitter has had appropriate setters added.
lib/check_container_test.go and cmd/check_container_test.go
I split these two files into files more geared towards their testing purposes. The cmd package one tests stuff that has to do with the cmd package, for example.

openshift-ci · 2022-10-20T22:00:57Z

Hi @sebrandon1. Thanks for your PR.

I'm waiting for a redhat-openshift-ecosystem member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dcibot · 2022-10-20T22:58:40Z

SUCCESS https://www.distributed-ci.io/jobs/67a66f18-027a-4581-822c-ddda74e890e7/jobStates
from Add lib to preflight #809

bcrochet · 2022-10-21T11:57:51Z

/ok-to-test

dcibot · 2022-10-21T13:37:25Z

SUCCESS https://www.distributed-ci.io/jobs/bd55371d-df1d-45de-9d34-18094856546f/jobStates
from Add lib to preflight #809

cmd/check_test.go

go.mod

lib/fakes.go

dcibot · 2022-10-21T19:05:08Z

SUCCESS https://www.distributed-ci.io/jobs/80c7feb6-35c3-4274-8264-abffe91f0522/jobStates
from Add lib to preflight #809

komish

@sebrandon1 Thanks a ton for the work on pushing this forward! I've added a few review comments, but I'll definitely need to take another pass. Before I do - discussion points ⬇️

We briefly discussed this when we met, but I wanted to go ahead and capture it here publicly so that we can have folks comment/provide feedback.

I think what I'm looking to see here out of the gate is the ability to run CheckContainer(...) and CheckOperator(...) for starters.

The inputs are up for debate (my ideal would be just a context and maybe your image string), but the outputs here would be results, and somehow, log files where relevant (TBD, but I'm putting that aside for now).

The goal here would be to make sure that folks starting to consume preflight as a library don't have to go through the hurdles to set up the PreflightCheck configurations, etc.

To that end, I'm thinking (sorry @bcrochet) that we start by moving most of this lib work into a top-level internal package, accessible by both cmd and everything else, wire up CheckOperator and CheckContainer as callable functions/methods, and then if we need to pull more things out of this internal library, for your use case (and the use case of others), then we'd do that as well.

If we did that, though, we want to make sure that the exported functions are actually what you need to benefit from calling the Preflight library in your use cases. Does that sound like something that would work for you?

@sebrandon1 @acornett21 @bcrochet WDYT?

lib/lib.go

lib/types.go

sebrandon1 · 2022-10-21T20:06:45Z

@komish Thanks for the review! Yes, actually what you propose makes a lot of sense by pushing some of that logic farther into the code and only getting two functions CheckContainer(...) and CheckOperator(...) with appropriate parameters exposed that would make things much easier. Those two functions could return your runtime.Results structs and handle everything internally versus me interpreting a JSON blob and unmarshalling it back into your structs. Removes a lot of complexity on the whoever "consumes" the lib so I'm fine with that.

With my initial PoC approach I didn't want to change too much to get this working but this makes more sense for future-proofing the effort. Very exciting!

komish · 2022-10-21T20:11:57Z

@sebrandon1 for sure. I’ll give some folks time to look and then we can chart the path to get this merged!

cmd/check_container_test.go

komish · 2022-10-24T19:09:15Z

@sebrandon1 I'll have you move lib to internal/lib, and then rewire the import statements to use that new path. From there, we'll expose hooks for you to be able to run the checks. This temporarily breaks your use case, but we'll fast-follow this on our end and I'll ping you into the PR that adds the exported functionality for your use case.

sebrandon1 · 2022-10-24T19:49:22Z

@komish Sounds excellent! I'll do that right now.

sebrandon1 · 2022-10-24T20:36:28Z

Okay in 7a84034 I moved the lib into a new "internal" folder in the top level and changed the references.

dcibot · 2022-10-24T20:43:45Z

FAILURE https://www.distributed-ci.io/jobs/1666b931-760e-4b62-8390-6dc61c3a3a7f/jobStates
from Add lib to preflight #809

komish · 2022-10-24T20:44:59Z

@sebrandon1

The CI failure reports:

internal/lib/lib.go:10                goimports  File is not `goimports`-ed with -local github.com/redhat-openshift-ecosystem/openshift-preflight
internal/lib/types.go:12              goimports  File is not `goimports`-ed with -local github.com/redhat-openshift-ecosystem/openshift-preflight
certification/internal/lib/lib.go:10  goimports  File is not `goimports`-ed with -local github.com/redhat-openshift-ecosystem/openshift-preflight

Given that's the only failure I see right at this moment, I think running this and committing the changes should fix the issue:

./bin/golangci-lint run --fix

sebrandon1 · 2022-10-24T20:54:53Z

Okay fixed. I ran ./bin/golangci-lint run --fix.

coveralls · 2022-10-24T21:12:04Z

Coverage decreased (-0.02%) to 84.48% when pulling 1bc39fc on sebrandon1:rename_lib into 2108a28 on redhat-openshift-ecosystem:main.

dcibot · 2022-10-24T21:16:52Z

FAILURE https://www.distributed-ci.io/jobs/1ba5f8d3-9d5a-44bd-aaf4-544aba81bd87/jobStates
from Add lib to preflight #809

dcibot · 2022-10-24T21:41:34Z

FAILURE https://www.distributed-ci.io/jobs/cd10419b-a0cd-41c0-8009-12bbed6190b5/jobStates
from Add lib to preflight #809

certification/runtime/config.go

dcibot · 2022-10-25T16:32:03Z

FAILURE https://www.distributed-ci.io/jobs/12842aeb-5eee-4ab2-8973-a78f8a98db62/jobStates
from Add lib to preflight #809

komish · 2022-10-25T17:11:42Z

Added on two commits to fix the coverage issues and fix a couple of case issues in comments for exported functions.

Coverage issues were fixed by:

adding suite_test.go to the internal/lib. They were being skipped.
moving NewCheckOperatorRunner-related tests to internal/lib where that function now lives
moving fakes.go to fakes_test.go which excludes it in coverage evaluation.

komish · 2022-10-25T17:22:26Z

0d08e68 removes a duplicate bit of code and moves the test to the correct location.

komish · 2022-10-25T17:34:14Z

@sebrandon1 @bcrochet @acornett21 as I've made changes here, PTAL.

dcibot · 2022-10-25T17:34:25Z

FAILURE https://www.distributed-ci.io/jobs/84825c44-a20d-4c21-845d-0aec82dd6322/jobStates
from Add lib to preflight #809

komish · 2022-10-25T17:46:19Z

FAILURE https://www.distributed-ci.io/jobs/84825c44-a20d-4c21-845d-0aec82dd6322/jobStates
from Add lib to preflight #809

@tkrishtop Just a heads up that it seems like this is a problem in the infrastructure's ability to RW files on the filesystem (permission denied) and doesn't seem related to the PR's changes at first glance.

komish

I've made changes so I'm removing my request for change and requesting reviews

contributed changes

acornett21 · 2022-10-25T20:55:00Z

@komish I think this addresses everything, the only thing we'd need is the commits squashed.

dcibot · 2022-10-25T21:18:35Z

FAILURE https://www.distributed-ci.io/jobs/926f7a49-8de3-429e-97b1-0c4c86d991f0/jobStates
from Add lib to preflight #809

bcrochet

/lgtm

openshift-ci · 2022-10-25T21:19:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, sebrandon1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [acornett21,bcrochet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from komish and tonytcampbell October 20, 2022 22:00

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 20, 2022

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2022

sebrandon1 force-pushed the rename_lib branch from 929fb63 to fd8ff7f Compare October 21, 2022 13:16

acornett21 requested changes Oct 21, 2022

View reviewed changes

cmd/check_test.go Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

lib/fakes.go Outdated Show resolved Hide resolved

openshift-ci bot assigned acornett21 Oct 21, 2022

sebrandon1 force-pushed the rename_lib branch 3 times, most recently from 1692109 to c7b8663 Compare October 21, 2022 18:45

komish previously requested changes Oct 21, 2022

View reviewed changes

openshift-ci bot assigned komish Oct 21, 2022

komish reviewed Oct 24, 2022

View reviewed changes

cmd/check_container_test.go Show resolved Hide resolved

sebrandon1 mentioned this pull request Oct 24, 2022

Use constructor-style for NoopSubmitter #810

Merged

sebrandon1 force-pushed the rename_lib branch from 7a84034 to 50ea3fe Compare October 24, 2022 20:53

sebrandon1 force-pushed the rename_lib branch from 50ea3fe to 8dd7fbc Compare October 24, 2022 21:04

sebrandon1 force-pushed the rename_lib branch from 8dd7fbc to 5105ed2 Compare October 24, 2022 21:24

komish reviewed Oct 25, 2022

View reviewed changes

certification/runtime/config.go Outdated Show resolved Hide resolved

komish reviewed Oct 25, 2022

View reviewed changes

sebrandon1 force-pushed the rename_lib branch from 0d08e68 to f1a88e5 Compare October 25, 2022 21:01

acornett21 approved these changes Oct 25, 2022

View reviewed changes

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 25, 2022

Add lib to preflight

1bc39fc

sebrandon1 force-pushed the rename_lib branch from f1a88e5 to 1bc39fc Compare October 25, 2022 21:05

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2022

bcrochet approved these changes Oct 25, 2022

View reviewed changes

openshift-ci bot assigned bcrochet Oct 25, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2022

acornett21 merged commit 7a7320f into redhat-openshift-ecosystem:main Oct 25, 2022

sebrandon1 deleted the rename_lib branch October 25, 2022 21:39

sebrandon1 mentioned this pull request Oct 26, 2022

Add preflight-lib reference test-network-function/cnf-certification-test#592

Closed

naemono mentioned this pull request Nov 29, 2022

Exposing ability to Submit results when used as a library. #845

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lib to preflight #809

Add lib to preflight #809

sebrandon1 commented Oct 20, 2022 •

edited

Loading

openshift-ci bot commented Oct 20, 2022

dcibot commented Oct 20, 2022

bcrochet commented Oct 21, 2022

dcibot commented Oct 21, 2022

dcibot commented Oct 21, 2022

komish left a comment

sebrandon1 commented Oct 21, 2022

komish commented Oct 21, 2022

komish commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

dcibot commented Oct 24, 2022

komish commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

coveralls commented Oct 24, 2022 •

edited

Loading

dcibot commented Oct 24, 2022

dcibot commented Oct 24, 2022

dcibot commented Oct 25, 2022

komish commented Oct 25, 2022

komish commented Oct 25, 2022

komish commented Oct 25, 2022

dcibot commented Oct 25, 2022

komish commented Oct 25, 2022

komish left a comment

acornett21 commented Oct 25, 2022

dcibot commented Oct 25, 2022

bcrochet left a comment

openshift-ci bot commented Oct 25, 2022

Add lib to preflight #809

Add lib to preflight #809

Conversation

sebrandon1 commented Oct 20, 2022 • edited Loading

openshift-ci bot commented Oct 20, 2022

dcibot commented Oct 20, 2022

bcrochet commented Oct 21, 2022

dcibot commented Oct 21, 2022

dcibot commented Oct 21, 2022

komish left a comment

Choose a reason for hiding this comment

sebrandon1 commented Oct 21, 2022

komish commented Oct 21, 2022

komish commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

dcibot commented Oct 24, 2022

komish commented Oct 24, 2022

sebrandon1 commented Oct 24, 2022

coveralls commented Oct 24, 2022 • edited Loading

dcibot commented Oct 24, 2022

dcibot commented Oct 24, 2022

dcibot commented Oct 25, 2022

komish commented Oct 25, 2022

komish commented Oct 25, 2022

komish commented Oct 25, 2022

dcibot commented Oct 25, 2022

komish commented Oct 25, 2022

komish left a comment

Choose a reason for hiding this comment

acornett21 commented Oct 25, 2022

dcibot commented Oct 25, 2022

bcrochet left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 25, 2022

sebrandon1 commented Oct 20, 2022 •

edited

Loading

coveralls commented Oct 24, 2022 •

edited

Loading