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

workflows: add commitlint workflow #92

Closed
wants to merge 1 commit into from

Conversation

obnoxxx
Copy link
Collaborator

@obnoxxx obnoxxx commented Jan 16, 2023

component definitions are basically taken from the existing git log.

Signed-off-by: Michael Adam obnox@redhat.com

@obnoxxx obnoxxx force-pushed the add-commitlint branch 2 times, most recently from d581a94 to 8f877ad Compare January 16, 2023 15:05
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 16, 2023

strange, I don't see why the new workflow file does not trigger a check run ... 🤷‍♂️

component definitions are basically taken from the existing git log.

Signed-off-by: Michael Adam <obnox@samba.org>
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 16, 2023

strange, I don't see why the new workflow file does not trigger a check run ... 🤷‍♂️

now it starts and even succeeds! 😄

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

The type-enum looks ok, but see other comments in this PR

@@ -0,0 +1,31 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The recommended method is to use javascript config file. See: https://commitlint.js.org/#/reference-configuration

"workflows",
"kubernetes",
"hack",
"Makefile"
Copy link
Collaborator

@synarete synarete Jan 16, 2023

Choose a reason for hiding this comment

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

We use makefile: (lowercase) labels so far. Better keep it that way.

@@ -0,0 +1,26 @@
name: Commitlint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unlike rook, in samba-operator we use single workflow-action yaml, which allows us to see all jobs at once (as DAG) and halt workflow as soon a a job fails. Better have this section part of main.yaml with proper needs.


jobs:
lint:
runs-on: ubuntu-18.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use ubuntu-latest

lint:
runs-on: ubuntu-18.04
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this env? I think its redundant and confusing.

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: actions/checkout@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better use latest actions/checkout@v3

steps:
- uses: actions/checkout@v2
with:
fetch-depth: 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this? AFAIK, it is not needed in this case.

- uses: actions/checkout@v2
with:
fetch-depth: 0
- uses: wagoid/commitlint-github-action@v2.0.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use latest version: wagoid/commitlint-github-action@v5

@obnoxxx obnoxxx marked this pull request as draft January 20, 2023 15:29
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 20, 2023

converted to draft becaus @phlogistonjohn suggested we solve the corresponding effort for samba-operator first : samba-in-kubernetes/samba-operator#272 or samba-in-kubernetes/samba-operator#274

@anoopcs9
Copy link
Collaborator

Support for gitlint is already merged via #124.

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.

None yet

3 participants