internal/scorecard/alpha: add parallelism support #3420
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
joelanford
force-pushed
the
scorecard-parallel
branch
5 times, most recently
from
ac7da05 to
9feca46
Compare
hack/boilerplate.go.txt
Outdated
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. |
There was a problem hiding this comment.
I'll remove this file once I know I no longer need to re-create the generated deep-copy files.
internal/registry/labels.go
Outdated
| // TODO: replace `gopkg.in/yaml.v2` with `sigs.k8s.io/yaml` once operator-registry has `json` tags in the | ||
| // annotations struct. |
There was a problem hiding this comment.
Looks like my imports sorter clobbered this. I'll add it back.
joelanford
force-pushed
the
scorecard-parallel
branch
from
a4706a9 to
447902a
Compare
joelanford
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
joelanford
force-pushed
the
scorecard-parallel
branch
5 times, most recently
from
71785e9 to
dc3d55e
Compare
There was a problem hiding this comment.
So, you didn't change this is this PR but this seems as good a place as any to discuss it. Why do we fatally exit out in places like here instead of logging that something went wrong, and returning an error? This to me to not be very go-like behavior and is more similar to panicing, which I think should be more of a last resort. I ask because I've encountered a bunch of lines like this in my attempts to test out the cmd layer and it's difficult to test such behavior.
| } | ||
|
|
||
| if hasFailingTest(scorecardTests) { | ||
| os.Exit(1) |
There was a problem hiding this comment.
Should the operator-sdk process return a non-zero exit code if it ran successfully but a test failed? 🤔 The cli command executed successfully. In either case, this is different from the previous behavior.
There was a problem hiding this comment.
upon poking around some test frameworks, I guess this is the usual behavior. good catch, although I wonder if it would be more golike to simply return an error.
| ) | ||
|
|
||
| // TODO(joelanford): rewrite to use ginkgo/gomega |
There was a problem hiding this comment.
Yeah! I can answer any questions you might have about ginkgo/gomega.
- Validate RegistryPod struct fields and set defaults - Verify pod creation using polling and ensuring pod status is Running - Add logic to check if pod already exists and return existing pod - Add *corev1.Pod as an unexported field to RegistryPod struct - Make kubernetes client exported on RegistryPod so the caller can set it - Add templating for container command - Add logic to wrap pod logs inside error in case of failures - Use k8sutil's TrimDNS1123Label func to construct pod name - Add unit tests in ginkgo/gomega
|
@jberkhahn Typically, we try to distinguish between between a usage error (e.g. with invalid flags or arguments) versus an error that occurred during the actual execution of the command. With cobra (at least as far as we've been able to ascertain), returning an error from |
|
This PR got a bit bigger as I found other improvements to make, so I'm going to break this one up and update it to be just the parallelism feature. |
Description of the change:
Motivation for the change:
Give users more flexbility in scheduling their tests and having them complete faster when they can be run in parallel.
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs