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

ci: Add a workflow to validate pull request titles #734

Merged
merged 6 commits into from
Aug 15, 2022

Conversation

shahzadlone
Copy link
Member

Relevant issue(s)

Resolves #718

Description

A GitHub workflow action that validates PR title.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Through the CI runs of this PR.

Specify the platform(s) on which this was tested:

  • WSL2 (Manjaro)

@shahzadlone shahzadlone added ci/build This is issue is about the build or CI system, and the administration of it. action/no-benchmark Skips the action that runs the benchmark. labels Aug 11, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.3.1 milestone Aug 11, 2022
@shahzadlone shahzadlone self-assigned this Aug 11, 2022
@shahzadlone shahzadlone changed the title wip: Add a workflow to validate pull request titles. ci: Add a workflow to validate pull request titles. Aug 11, 2022
@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch 2 times, most recently from 32c951a to 66ad59c Compare August 11, 2022 07:15
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles. ci: Add a workflow to validate pull request titles asd as asd asd as a Aug 11, 2022
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles asd as asd asd as a ci: Add a workflow to validate pull request titles asd as asd asd as aa Aug 11, 2022
@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch from 66ad59c to 324998d Compare August 11, 2022 07:23
@shahzadlone shahzadlone requested a review from a team August 11, 2022 07:24
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles asd as asd asd as aa ci: Add a workflow to validate pull request title Aug 11, 2022
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request title ci: Add a workflow to validate pull request title. Aug 11, 2022
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request title. ci: Add a workflow to validate pull request titles Aug 11, 2022
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles ci: Add a workflow to validate pull request titles Aug 11, 2022
@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch from 324998d to f96ad1f Compare August 11, 2022 07:44
@orpheuslummis
Copy link
Contributor

running the script through shellcheck gives the following advice:

Line 47:
printf "Info: label = '${LABEL}'\n";
       ^-- SC2059 (info): Don't use variables in the printf format string. Use printf '..%s..' "$foo".
 
Line 48:
printf "Info: description = '${DESCRIPTION}'\n";
       ^-- SC2059 (info): Don't use variables in the printf format string. Use printf '..%s..' "$foo".
 
Line 79:
for validLabel in ${VALID_LABELS[@]}; do
                  ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements.

https://www.shellcheck.net/

tools/scripts/validate-conventional-style.sh Outdated Show resolved Hide resolved
exit 7;
fi

# Validate that the last character of the description is a lower case alphabet character.
Copy link
Contributor

Choose a reason for hiding this comment

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

question: that would exclude '.' at the end right?

Copy link
Member Author

@shahzadlone shahzadlone Aug 11, 2022

Choose a reason for hiding this comment

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

Yea so a PR title with . (non-lowercase-alphabet) in the end will fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this means that we want out PR titles to never have a dot at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

So this means that we want out PR titles to never have a dot at the end?

That is what this action would enforce, was that not the consensus?

tools/scripts/validate-conventional-style.sh Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch 2 times, most recently from 95f50ac to cb63fe8 Compare August 12, 2022 03:26
@shahzadlone
Copy link
Member Author

running the script through shellcheck gives the following advice:

Line 47:
printf "Info: label = '${LABEL}'\n";
       ^-- SC2059 (info): Don't use variables in the printf format string. Use printf '..%s..' "$foo".
 
Line 48:
printf "Info: description = '${DESCRIPTION}'\n";
       ^-- SC2059 (info): Don't use variables in the printf format string. Use printf '..%s..' "$foo".
 
Line 79:
for validLabel in ${VALID_LABELS[@]}; do
                  ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements.

https://www.shellcheck.net/

All fixed.

@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles ci: testAdd a workflow to validate pull request titles Aug 12, 2022
@shahzadlone shahzadlone changed the title ci: testAdd a workflow to validate pull request titles ci: Add a workflow to validate pull request titles Aug 12, 2022
@orpheuslummis
Copy link
Contributor

Screen Shot 2022-08-12 at 09 32 25

question: that would also apply to these kinds of commits on master? if so, how could we name these merge commits? merge: xyz ?

@orpheuslummis
Copy link
Contributor

Screen Shot 2022-08-12 at 09 32 25

question: that would also apply to these kinds of commits on master? if so, how could we name these merge commits? merge: xyz ?

answer: nope, Validate Title Workflow only applies on develop`

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

This is what I get when I try to run the test:

fredcarle@Freds-MBP defradb % make test:scripts
./scripts_test.sh
PASS
PASS
PASS
PASS
FAIL ...
> Command  : [./validate-conventional-style.sh 'feat: a']
> Expected : [5]
> ACTUAL   : [4]
make[1]: *** [test] Error 1

That's because readarray isn't installed by default on macOS (and probably others). See my suggestion bellow.

tools/scripts/scripts_test.sh Outdated Show resolved Hide resolved
exit 7;
fi

# Validate that the last character of the description is a lower case alphabet character.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this means that we want out PR titles to never have a dot at the end?

tools/scripts/validate-conventional-style.sh Outdated Show resolved Hide resolved
tools/scripts/validate-conventional-style.sh Outdated Show resolved Hide resolved
tools/scripts/scripts_test.sh Show resolved Hide resolved
@shahzadlone
Copy link
Member Author

That's because readarray isn't installed by default on macOS (and probably others). See my suggestion bellow.

Solved using sed, LMK if it works for macOS now.

@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch from 5022078 to ea9b267 Compare August 15, 2022 10:39
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM!

.github/workflows/validate-title.yml Outdated Show resolved Hide resolved
@shahzadlone shahzadlone changed the title ci: Add a workflow to validate pull request titles ci:Add a workflow to validate pull request titles Aug 15, 2022
@shahzadlone shahzadlone changed the title ci:Add a workflow to validate pull request titles ci: add a workflow to validate pull request titles Aug 15, 2022
@shahzadlone shahzadlone changed the title ci: add a workflow to validate pull request titles ci: Add a workflow to validate pull request titles Aug 15, 2022
@shahzadlone shahzadlone force-pushed the lone/ci/add-conventional-commit-script branch from ea9b267 to 267f4da Compare August 15, 2022 22:19
@shahzadlone shahzadlone merged commit c052394 into develop Aug 15, 2022
@shahzadlone shahzadlone deleted the lone/ci/add-conventional-commit-script branch August 15, 2022 22:29
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
- Resolves sourcenetwork#718 

A GitHub workflow action that validates PR title.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. ci/build This is issue is about the build or CI system, and the administration of it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GitHub Action to check if conventional commits is adhered to
3 participants