You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/04/20 05:50:54 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
michaeljmarshall opened a new pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276
### Motivation
Currently, when there are changes made to code and documentation in the same PR, many of the tests are skipped because the conditional logic is not being parsed as expected.
@jerrypeng noticed this issue on this PR: https://github.com/apache/pulsar/pull/10254.
### Modifications
Fix the usage of `${{}}`.
### Verifying this change
I ran tests in my personal GitHub repository to validate this change. Here is the PR that shows it: https://github.com/michaeljmarshall/pulsar/pull/2/checks?check_run_id=2387660628. I used echo to validate what changed. (Note that the PR uses the base of https://github.com/apache/pulsar/pull/10254 to reproduce the problem.)
### Does this pull request potentially affect one of the following parts:
This affects all tests that run for PRs that include changes to documentation and code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823006051
> I added comments to this staging PR [michaeljmarshall#2](https://github.com/michaeljmarshall/pulsar/pull/2) to show where and how I tested the changes.
Yes, really good. I checked that while reviewing.
> I haven't been able to find explicit documentation on this specific case of interpolation yet. However, it does seem that this change produces the expected outcome for the affected tests.
The documentation is here: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#about-contexts-and-expressions . However there isn't a clear answer.
What seems to happen is that when using this syntax:
```
if: ${{ steps.changes.outputs.all_count > steps.changes.outputs.docs_count }}
```
the values are compared as string values and not numeric values.
In the example case, `"40" > "7"` is false.
When using the other syntax, the explicit expressions `${{ .... }}` will trigger the autodetection of the value. This converts them into numbers
```
if: ${{ steps.changes.outputs.all_count }} > ${{ steps.changes.outputs.docs_count }}
```
Now `40 > 7` is true.
There is no documentation about how to explicitly convert value to a number. I believe that your fix is the correct way to do it. Thanks for saving my day @michaeljmarshall !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823017306
Just sharing more from the documentation: It that seems [`fromJSON`](https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#fromjson) function would also be a way to parse a number in a string as a number.
something like this could possibly work too. I think this is very verbose compared to the solution in this PR:
```
if: ${{ fromJSON(steps.changes.outputs.all_count) > fromJSON(steps.changes.outputs.docs_count) }}
```
`fromJSON("40") > fromJSON("7")` -> `40 > 7` -> `true`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823014236
@lhotari thank you for finding and sharing the documentation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823002221
@lhotari thanks!
I added comments to this staging PR https://github.com/michaeljmarshall/pulsar/pull/2 to show where and how I tested the changes.
I haven't been able to find explicit documentation on this specific case of interpolation yet. However, it does seem that this change produces the expected outcome for the affected tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823017306
Just sharing more from the documentation: It that seems [`fromJSON`](https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#fromjson) function would also be a way to parse a number in a string as a number.
something like this could possibly work too.
```
if: ${{ fromJSON(steps.changes.outputs.all_count) > fromJSON(steps.changes.outputs.docs_count) }}
```
(I think this is very verbose compared to the solution in this PR)
`fromJSON("40") > fromJSON("7")` -> `40 > 7` -> `true`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-823008631
> There is no documentation about how to explicitly convert value to a number. I believe that your fix is the correct way to do it.
I found the documentation. 'Literals': https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#literals . The fix is the correct way to convert to numbers for comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-822996370
Thank you @michaeljmarshall for investigating and debugging this. I'll review this soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #10276: [GitHub Actions] Fix logic that was incorrectly skipping tests
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10276:
URL: https://github.com/apache/pulsar/pull/10276#issuecomment-822993162
@lhotari, @sijie, @eolivelli - PTAL. This is an urgent fix that should be merged as soon as we're good with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org