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

Add PR checks via GitHub actions #18

Closed
ckadner opened this issue Mar 1, 2024 · 7 comments · Fixed by #256
Closed

Add PR checks via GitHub actions #18

ckadner opened this issue Mar 1, 2024 · 7 comments · Fixed by #256
Assignees
Labels

Comments

@ckadner
Copy link
Contributor

ckadner commented Mar 1, 2024

GitHub actions along with concise Bash scripts could/should be employed to make sure the submitted "qna.yml" and "knowledge.md" files are formatted correctly (style and content structure)

@ckadner
Copy link
Contributor Author

ckadner commented Mar 1, 2024

I'd be happy to take a first stab at it :-)

@ckadner ckadner self-assigned this Mar 1, 2024
@oindrillac
Copy link
Contributor

Checks on PR would be useful. Would reduce the reliance on reviewers and the error handling on the processing backend to weed out incorrectly formatted yamls.

@jeremyeder
Copy link
Member

@ckadner please do it -- it should be possible for users to re-run those checks by using slash-commands against our github bot. @Tomcli did that, hopefully he can help integrate (e.g. /test)

@Tomcli
Copy link
Contributor

Tomcli commented Mar 4, 2024

Github actions will rerun when a new commit is pushed in the PR. To enable retest command on prow, we need to deploy the full prow cluster. The github action version of prow only support the following commands:
Available github action prow commands: https://github.com/jpmcb/prow-github-actions/blob/main/docs/commands.md
Full prow feature for retest github actions: kubernetes/test-infra#31132

@wking
Copy link
Contributor

wking commented Mar 4, 2024

sounds like there is some CLI-side work in this space: instructlab/instructlab#133 and instructlab/instructlab#205.

@ckadner ckadner added the test label Mar 5, 2024
@iranzo
Copy link
Contributor

iranzo commented Mar 6, 2024

#60 does some checking automatically, we need to just enable the CI run of precommit on the new PR's via #93

@ckadner
Copy link
Contributor Author

ckadner commented Mar 6, 2024

I made the DCO check mandatory and added a simple YAML linter for qna.yaml files yesterday 2a15c43


An example of linter completing successfully in a few seconds:

image

An example of linter catching formatting errors:

image

Which also shows the offensive lines in the Files changed view for reviewers:

image

My intention is to re-use what @lehors has code in instructlab/instructlab#133 and instructlab/instructlab#205 in a second iteration.

ckadner added a commit that referenced this issue Mar 8, 2024
The current CI checks do a basic `yamllint` but don't actually verify
the `qna.yaml` files contain the required contents

This PR updates the existing CI lint workflow:
- Only run the linter when `qna.yaml` file(s) were changed
- Add a step to identify only the `qna.yaml` file(s) changed in the PR
- Use `yq` to check specific fields inside the `qna.yaml` file(s)
- Generate a match-able log messages for missing/empty fields
- Use a problem-matcher to parse the generate error messages
  to annotate the Changed files in the PR
- Generate an error for knowledge contributions

Resolves #18

---------

Signed-off-by: Christian Kadner <[email protected]>
@luke-inglis luke-inglis added ci and removed test labels Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants