Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

fix: Reformatted file to comply with yamllint reqs#3115

Merged
mergify[bot] merged 2 commits intoinstructlab:maininstructlab/instructlab:mainfrom
jpodivin:fix/yamllintjpodivin/instructlab:fix/yamllintCopy head branch name to clipboard
Feb 3, 2025
Merged

fix: Reformatted file to comply with yamllint reqs#3115
mergify[bot] merged 2 commits intoinstructlab:maininstructlab/instructlab:mainfrom
jpodivin:fix/yamllintjpodivin/instructlab:fix/yamllintCopy head branch name to clipboard

Conversation

@jpodivin
Copy link
Contributor

@jpodivin jpodivin commented Feb 2, 2025

This makes locally running newly introduced yamllint possible, while also resolving an ongoing CI issue.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

Signed-off-by: Jiri Podivin <jpodivin@gmail.com>
@mergify mergify bot added the testing Relates to testing label Feb 2, 2025
Without this change the yamllint couldn't be executed
in developer environment where tox was run before.

Signed-off-by: Jiri Podivin <jpodivin@gmail.com>
@reidliu41 reidliu41 requested a review from jwm4 February 2, 2025 12:54
Copy link
Contributor

@jwm4 jwm4 left a comment

Choose a reason for hiding this comment

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

The change to qna.yaml looks good to me; sorry for the problem with it! I don't understand the .yamllint.yaml change, but I have no objection to it.

@jwm4 jwm4 requested a review from a team February 2, 2025 13:48
@jwm4
Copy link
Contributor

jwm4 commented Feb 2, 2025

Note that this resolves #3114

@jpodivin
Copy link
Contributor Author

jpodivin commented Feb 2, 2025

The change to qna.yaml looks good to me; sorry for the problem with it! I don't understand the .yamllint.yaml change, but I have no objection to it.

Basically, .tox dir can end up filled with various yamls that are incompatible with the yamllint requirements.
By adding the path to ignore list, we make sure that they don't trigger an error.

@jwm4
Copy link
Contributor

jwm4 commented Feb 2, 2025

That makes sense. Thx for explaining!

@nathan-weinberg nathan-weinberg linked an issue Feb 3, 2025 that may be closed by this pull request
@nathan-weinberg
Copy link
Member

@nathan-weinberg nathan-weinberg requested a review from a team February 3, 2025 06:59
@mergify mergify bot added the one-approval PR has one approval from a maintainer label Feb 3, 2025
@mergify mergify bot merged commit 5212509 into instructlab:main Feb 3, 2025
5 checks passed
@mergify mergify bot removed the one-approval PR has one approval from a maintainer label Feb 3, 2025
@reidliu41
Copy link
Contributor

thank you so much..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

yamllint check failed at the test job

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.