Skip to content

Conversation

@leseb
Copy link
Collaborator

@leseb leseb commented May 6, 2025

This CI check replaces the linter action, it has been included into the pre-commit hook.
The hook has 2 specialized ones:

  • the go linter
  • the generate manifest artifacts

You can run pre-commit locally:

pre-commit run --all-files

You can also install it so it will run before committing code.

pre-commit install

@leseb
Copy link
Collaborator Author

leseb commented May 6, 2025

I think there is a back and forth between our local golang versions. @VaishnaviHire

@leseb leseb requested review from VaishnaviHire and rhuss May 6, 2025 15:04
@VaishnaviHire
Copy link
Collaborator

Yeah looks like it. Maybe just updating the go-lint version should suffice.

@rhuss
Copy link
Collaborator

rhuss commented May 6, 2025

Looks good to me. Could you briefly elaborate the benefit of pre-commit in addition to make lint as this is an additional development dependency that needs to be installed ?

Another idea, would to describe in CONTRIBUTING.md how to install it as a local pre-commit hook for git, so that it is automatically called on a git commit (or is this too much and we want to keep more control when to run the hook ?). Maybe there's something like a 'pre push' hook ?

Another comment is about the role.yaml file as it still looks weird to me. I wonder whether we should careful go through it and drill it down to the stuff that we really need. E.g. we don't need to list Services, or do we ? But that's a question for another PR.

@leseb
Copy link
Collaborator Author

leseb commented May 6, 2025

Looks good to me. Could you briefly elaborate the benefit of pre-commit in addition to make lint as this is an additional development dependency that needs to be installed ?

As explained in the CONTRIBUTING guide, installing pre-commit locally is optional, it's up to the developers. But it's nice to have a common tool to run locally what the CI does in an all in one fashion. Also, when setup correctly, it will prevent developers from pushing inadequate content and save CI cycles.

Another idea, would to describe in CONTRIBUTING.md how to install it as a local pre-commit hook for git, so that it is automatically called on a git commit (or is this too much and we want to keep more control when to run the hook ?). Maybe there's something like a 'pre push' hook ?

This is what pre-commit install does, it configure it as a git hook to run on every commit.
I personally use it as a pre-push but again, it's up to the developer's preference.

Another comment is about the role.yaml file as it still looks weird to me. I wonder whether we should careful go through it and drill it down to the stuff that we really need. E.g. we don't need to list Services, or do we ? But that's a question for another PR.

I have the same feeling, but all those things were auto-generated :(

This CI check replaces the linter action, it has been included into the
pre-commit hook.
The hook has 2 specialized ones:

* the go linter
* the generate manifest artifacts

You can run pre-commit locally:

```
pre-commit run --all-files
```

You can also install it so it will run before committing code.

```
pre-commit install
```

Signed-off-by: Sébastien Han <seb@redhat.com>
@leseb leseb requested a review from rhuss May 6, 2025 20:03
@rhuss
Copy link
Collaborator

rhuss commented May 7, 2025

Thanks @leseb for the elaboration, looks good to me!

For the role issue I've opened #18

/lgtm

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

lgtm

@leseb leseb merged commit 5d44a65 into llamastack:main May 7, 2025
1 check passed
@leseb leseb deleted the pre-commit branch May 7, 2025 07:29
VaishnaviHire added a commit to VaishnaviHire/llama-stack-k8s-operator that referenced this pull request Jul 16, 2025
VaishnaviHire pushed a commit to VaishnaviHire/llama-stack-k8s-operator that referenced this pull request Jul 22, 2025
…r digest to 6d5a657 (llamastack#16)

Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
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