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

feat: add ability to fail on ERR and other fail-on statuses for breaking changes #43

Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,31 @@ jobs:
fail-on-diff: true
deprecation-days-beta: 14
deprecation-days-stable: 21
oasdiff_breaking_fail_on_explicit:
runs-on: ubuntu-latest
name: Test breaking changes fail-on
steps:
- name: checkout
uses: actions/checkout@v4
- name: Running breaking action
id: test_breaking_changes
uses: ./breaking
with:
base: 'specs/base.yaml'
revision: 'specs/revision-breaking.yaml'
fail-on-diff: false
fail-on: ERR
- name: Test breaking changes action output
run: |
delimiter=$(cat /proc/sys/kernel/random/uuid | tr -d '-')
output=$(cat <<-$delimiter
${{ steps.test_breaking_changes.outputs.breaking }}
$delimiter
)
if [ "$output" != "1 breaking changes: 1 error, 0 warning" ]; then
echo "Expected output '6 breaking changes: 2 error, 4 warning' but got '$output'" >&2
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

What you are testing?
I suggest taking an example that there is warnings but not errors and set fail-on: ERR and it should not fail

oasdiff_changelog:
runs-on: ubuntu-latest
name: Test generation of changelog
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,14 @@ Additional arguments:
| CLI | Action input | Default |
|---------------------------|-------------------------|---------|
| --fail-on WARN | fail-on-diff | true |
| --fail-on | fail-on | '' |
| --include-checks | include-checks | csv |
| --include-path-params | include-path-params | false |
| --deprecation-days-beta | deprecation-days-beta | 31 |
| --deprecation-days-stable | deprecation-days-stable | 180 |
| --exclude-elements | exclude-elements | '' |

This action delivers a summary of breaking changes, accessible as a GitHub step output named `breaking`.
This action delivers a summary of breaking changes, accessible as a GitHub step output named `breaking`. Note: `fail-on-diff` takes precedence over `fail-on` when `fail-on-diff` is `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets align with oasdiff by having only fail-on. please remove fail-on-diff, we can achieve this by using fail-on: WARN


### Generate a changelog
Copy and paste the following snippet into your build .yml file:
Expand Down
5 changes: 5 additions & 0 deletions breaking/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ inputs:
description: 'Fail with exit code 1 if any breaking changes are found'
required: false
default: 'true'
fail-on:
description: 'Fail with exit code 1 if breaking changes are found at a particular status or severity level, e.g. ERR'
required: false
default: ''
include-checks:
description: 'Include any of the defined optional breaking changes checks'
required: false
Expand Down Expand Up @@ -38,6 +42,7 @@ runs:
- ${{ inputs.base }}
- ${{ inputs.revision }}
- ${{ inputs.fail-on-diff }}
- ${{ inputs.fail-on }}
- ${{ inputs.include-checks }}
- ${{ inputs.include-path-params }}
- ${{ inputs.deprecation-days-beta }}
Expand Down
15 changes: 9 additions & 6 deletions breaking/entrypoint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@ set -e
readonly base="$1"
readonly revision="$2"
readonly fail_on_diff="$3"
readonly include_checks="$4"
readonly include_path_params="$5"
readonly deprecation_days_beta="$6"
readonly deprecation_days_stable="$7"
readonly exclude_elements="$8"
readonly fail_on="$4"
readonly include_checks="$5"
readonly include_path_params="$6"
readonly deprecation_days_beta="$7"
readonly deprecation_days_stable="$8"
readonly exclude_elements="$9"

echo "running oasdiff breaking... base: $base, revision: $revision, fail_on_diff: $fail_on_diff, include_checks: $include_checks, include_path_params: $include_path_params, deprecation_days_beta: $deprecation_days_beta, deprecation_days_stable: $deprecation_days_stable, exclude_elements: $exclude_elements"
echo "running oasdiff breaking... base: $base, revision: $revision, fail_on_diff: $fail_on_diff, fail_on: $fail_on, include_checks: $include_checks, include_path_params: $include_path_params, deprecation_days_beta: $deprecation_days_beta, deprecation_days_stable: $deprecation_days_stable, exclude_elements: $exclude_elements"

# Build flags to pass in command
flags=""
if [ "$fail_on_diff" = "true" ]; then
flags="${flags} --fail-on WARN"
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed

Copy link
Author

Choose a reason for hiding this comment

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

@effoeffi For #43 (comment), the reason it was not removed was that it would be a breaking change itself if it was removed from the action. If someone relies on the fail_on_diff and it is removed, their workflows will break... of course fail-on is more flexible for any status. Will update the pr to remove fail_on_diff though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@effoeffi For #43 (comment), the reason it was not removed was that it would be a breaking change itself if it was removed from the action. If someone relies on the fail_on_diff and it is removed, their workflows will break... of course fail-on is more flexible for any status. Will update the pr to remove fail_on_diff though.

@jgill-shipwell that is why we are creating a release, and you should rely on it to avoid any breaking changes.

Copy link
Author

Choose a reason for hiding this comment

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

@effoeffi Updated this pr to remove fail_on_diff and match the fail-on param of oasdiff. Updated the tests, etc.

elif [ -z "$fail_on" ]; then
flags="${flags} --fail-on $fail_on"
fi
if [ "$include_path_params" = "true" ]; then
flags="${flags} --include-path-params"
Expand Down
Loading