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

jgill-shipwell · 2024-02-28T22:17:29Z

Adds the ability to fail the action/build on other statuses such as ERR instead of just WARN.

effoeffi · 2024-03-01T11:12:17Z

breaking/entrypoint.sh

 if [ "$fail_on_diff" = "true" ]; then
    flags="${flags} --fail-on WARN"
+elif [ "$fail_on_diff" != "false" ]
+    flags="${flags} --fail-on $fail_on_diff"


I don't think this is a best practice.

@effoeffi Is there a way to indicate in the oasdiff breaking change action to only fail on ERR instead of WARN?

This was one way to support/passthrough any status or error level without hardcoding the options in the action:

oasdiff-action/breaking/entrypoint.sh

Lines 17 to 20 in 6ddd3f7

if [ "$fail_on_diff" = "true" ]; then

flags="${flags} --fail-on WARN"

elif [ "$fail_on_diff" != "false" ]

flags="${flags} --fail-on $fail_on_diff"

I understand, but it is not best practice to use 'var' as both a boolean and a string. I suggest converting it to a string similar to oasdiff. Additionally, update the documentation, defaults, and create tests accordingly. Please also review other parameters for similar improvements.

@effoeffi Updated the action, README, etc. in oasdiff-action in d96dd2c.

@effoeffi Added a test in 468ff61.

tcooper-uk · 2024-03-06T20:07:25Z

Taken from the oasdiff help:

-o, --fail-on string                 exit with return code 1 when output includes errors with this level or higher: ERR or WARN

This means that the behaviour you have described already exists, failing on WARN will also fail on ERR.

Validated this is true with:

oasdiff breaking https://raw.githubusercontent.com/Tufin/oasdiff/main/data/openapi-test1.yaml https://raw.githubusercontent.com/Tufin/oasdiff/main/data/openapi-test4.yaml --fail-on WARN || echo $?

jgill-shipwell · 2024-03-06T20:11:06Z

@tcooper-uk Re: #43 (comment) We want to fail on ERR, but not fail on WARN as the warnings are allowed and should not break the build in our use case so the fail-on-diff that sets the command as fail-on WARN to break the build/fail the action did not work for this use case.

effoeffi · 2024-03-07T13:31:33Z

.github/workflows/test.yaml

+          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


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

effoeffi · 2024-03-07T13:34:25Z

README.md

 | --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`.


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

effoeffi · 2024-03-07T13:40:39Z

breaking/entrypoint.sh

 if [ "$fail_on_diff" = "true" ]; then
    flags="${flags} --fail-on WARN"


should be removed

@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.

@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.

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

… - updating tests, etc.

effoeffi · 2024-03-10T12:34:04Z

breaking/entrypoint.sh

+if [ -z "$fail_on" ]; then
+    flags="${flags} --fail-on $fail_on"
 fi


Do you want to check if it is not empty, right? -n?

effoeffi · 2024-03-10T12:42:52Z

.github/workflows/test.yaml

        with:
          base: 'specs/base.yaml'
          revision: 'specs/revision-breaking.yaml'
-          fail-on-diff: false
+          fail-on: 'WARN'
          output-to-file: 'breaking.txt'


Why this test do not fail?
oasdiff docs:

To exit with return code 1 when any breaking changes are found, WARN-level or ERR-level, add the --fail-on WARN flag.

Can you please explain the why you decided to change fail-on-diff: false to fail-on: 'WARN'?

effoeffi · 2024-03-10T12:49:02Z

README.md

+|---------------------------|-------------------------|---------|
+| --fail-on                 | fail-on                 | 'ERR'   |


The default oasdiff exit code is 0 if there is a breaking changes, why do you want to change that?

effoeffi · 2024-03-10T12:49:54Z

breaking/entrypoint.sh

@@ -31,12 +31,12 @@ readonly exclude_elements="$8"
 readonly composed="$9"
 readonly output_to_file="${10}"

-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, composed: $composed, output_to_file: $output_to_file"
+echo "running oasdiff breaking... base: $base, revision: $revision, 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"


why did you delete composed: $composed, output_to_file: $output_to_file"?

@jgill-shipwell could you please check this PR and let me know if it resolves your issue?

feat: add ability to fail on ERR and other fail-on statuses

6ddd3f7

jgill-shipwell force-pushed the feat/add-fail-on-error-other-cases branch from e18f685 to 6ddd3f7 Compare February 29, 2024 14:59

effoeffi reviewed Mar 1, 2024

View reviewed changes

feat: added breaking change error severity status flag

d96dd2c

jgill-shipwell force-pushed the feat/add-fail-on-error-other-cases branch 2 times, most recently from cf87137 to e19710d Compare March 1, 2024 20:31

fix: adding breaking change test

468ff61

jgill-shipwell force-pushed the feat/add-fail-on-error-other-cases branch from 98ab600 to 468ff61 Compare March 1, 2024 20:38

jgill-shipwell requested a review from effoeffi March 1, 2024 20:43

effoeffi reviewed Mar 7, 2024

View reviewed changes

fix: removing fail-on-diff and defaulting to oasdiff param of fail-on…

436f539

… - updating tests, etc.

jgill-shipwell force-pushed the feat/add-fail-on-error-other-cases branch from 7e5e023 to 436f539 Compare March 8, 2024 16:10

Merge branch 'main' into feat/add-fail-on-error-other-cases

c4ea527

jgill-shipwell requested a review from effoeffi March 8, 2024 16:13

effoeffi reviewed Mar 10, 2024

View reviewed changes

effoeffi closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jgill-shipwell commented Feb 28, 2024

effoeffi Mar 1, 2024

jgill-shipwell Mar 1, 2024 •

edited

Loading

effoeffi Mar 1, 2024

jgill-shipwell Mar 1, 2024 •

edited

Loading

jgill-shipwell Mar 1, 2024

tcooper-uk commented Mar 6, 2024

jgill-shipwell commented Mar 6, 2024 •

edited

Loading

effoeffi Mar 7, 2024

effoeffi Mar 7, 2024

effoeffi Mar 7, 2024

jgill-shipwell Mar 7, 2024

effoeffi Mar 7, 2024

jgill-shipwell Mar 8, 2024

effoeffi Mar 10, 2024

effoeffi Mar 10, 2024

effoeffi Mar 10, 2024

effoeffi Mar 10, 2024

effoeffi May 7, 2024 •

edited

Loading

	if [ "$fail_on_diff" = "true" ]; then
	flags="${flags} --fail-on WARN"
	elif [ "$fail_on_diff" != "false" ]
	flags="${flags} --fail-on $fail_on_diff"

		\|---------------------------\|-------------------------\|---------\|
		\| --fail-on \| fail-on \| 'ERR' \|

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

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

Conversation

jgill-shipwell commented Feb 28, 2024

Choose a reason for hiding this comment

jgill-shipwell Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgill-shipwell Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcooper-uk commented Mar 6, 2024

jgill-shipwell commented Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effoeffi May 7, 2024 • edited Loading

Choose a reason for hiding this comment

jgill-shipwell Mar 1, 2024 •

edited

Loading

jgill-shipwell Mar 1, 2024 •

edited

Loading

jgill-shipwell commented Mar 6, 2024 •

edited

Loading

effoeffi May 7, 2024 •

edited

Loading