Skip to content

[CI] Automate run pre-commit in workflow instead of manual#131

Merged
jgchn merged 1 commit into
llm-d-incubation:mainfrom
yankay:fix-enable
Oct 16, 2025
Merged

[CI] Automate run pre-commit in workflow instead of manual#131
jgchn merged 1 commit into
llm-d-incubation:mainfrom
yankay:fix-enable

Conversation

@yankay

@yankay yankay commented Oct 14, 2025

Copy link
Copy Markdown
Collaborator

In #123, a pre-commit error was introduced.

Therefore, this PR modifies the CI workflow to automatically run pre-commit , ref to https://github.com/llm-d-incubation/llm-d-infra/blob/main/.github/workflows/pre-commit.yaml . It also fixes this error by generating schema files during the pre-commit checks.

@yankay yankay force-pushed the fix-enable branch 3 times, most recently from b7e57ec to 35a5617 Compare October 14, 2025 12:19
@yankay

yankay commented Oct 14, 2025

Copy link
Copy Markdown
Collaborator Author

HI @JaredTan95 @jgchn

How do you think about the change :-)

Comment thread .github/workflows/pre-commit.yaml Outdated
shell: bash

- name: Run pre-commit
run: pre-commit run --show-diff-on-failure --color=always --verbose --all-files --show-diff-on-failure

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we should update the pre-commit-run Make target with the extra flags and call make pre-commit-run here?

.PHONY: pre-commit-run
Looks like a previous step could also benefit from make pre-commit-update?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @jgchn

It has been changed . :-)

The function of make pre-commit-update is to update pre-commit hooks, and it will modify the .pre-commit-config file.

…hema.json

Signed-off-by: Kay Yan <kay.yan@daocloud.io>
common: {}

# -- Usually used when using llm-d-modelservice as a subchart
enabled: true

@JaredTan95 JaredTan95 Oct 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we remove this? This field is mainly used as a condition in chart.yaml whcn as subchart, and by default, this field is set to true in the schema.json

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After communicating with @yankay , considering that the schema json is generated based on values.yaml, it can be left here

@jgchn jgchn merged commit f61f4ec into llm-d-incubation:main Oct 16, 2025
4 checks passed
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