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

kola: Move kola-denylist.yaml related sugar from cmd-kola to kola #2296

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

saqibali-2k
Copy link
Member

Before introducing more functionality to deny listed tests, we want to have the deny list logic contained in a single location (kola).

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Nice work @saqibali-2k. A few comments inline

@dustymabe
Copy link
Member

For your first commit message I'd break it up slightly:

Harness.go: Added functionality to parse deny listed tests from kola-denylist.yaml
cmd-kola: Removed the yaml parsing sugar

We try to have a single title line, then a blank line, then a description.

mantle/kola: add support for kola-denylist.yaml

Added functionality to parse deny listed tests from a `kola-denylist.yaml`
file if found. This moves the functionality from cmd-kola (python) into
kola proper (golang).

@saqibali-2k
Copy link
Member Author

We try to have a single title line, then a blank line, then a description.

Thanks for pointing that out! I'll try to follow that format from now on.

@saqibali-2k saqibali-2k force-pushed the kola-denylist-transfer-main branch 2 times, most recently from a660ddc to 99e00be Compare July 19, 2021 13:40
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

A few optional suggestions (comments).

@saqibali-2k saqibali-2k force-pushed the kola-denylist-transfer-main branch from 99e00be to 2f1a501 Compare July 19, 2021 15:41
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks a lot for tackling this! A few nits, but overall LGTM!

Added functionality to parse deny listed tests from a `kola-denylist.yaml`
file if found. This moves the functionality from cmd-kola (python) into
kola proper (golang).
@saqibali-2k saqibali-2k force-pushed the kola-denylist-transfer-main branch from 2f1a501 to 234b9fc Compare July 19, 2021 21:00
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe
Copy link
Member

/lgtm

@dustymabe
Copy link
Member

/override ci/prow/sanity

@openshift-ci
Copy link

openshift-ci bot commented Jul 19, 2021

@dustymabe: Overrode contexts on behalf of dustymabe: ci/prow/sanity

In response to this:

/override ci/prow/sanity

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.

@dustymabe
Copy link
Member

overriding the shellcheck CI issue that's been plaguing us.

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.

3 participants