Skip to content

Conversation

B-a-S
Copy link
Contributor

@B-a-S B-a-S commented Sep 5, 2023

Prevent runs NVIDIA_CI in case triggered by PR in the fork ( checking repo name)

It looks like https://github.com/B-a-S/ompi/actions/runs/6087266827

@B-a-S B-a-S force-pushed the main branch 2 times, most recently from 2925948 to fbe476d Compare September 5, 2023 16:45
needs: [deployment, build]
runs-on: [self-hosted, linux, x64, nvidia]
steps:
- name: Running tests
run: /start test
clean:
if: ${{ always() }}
if: (github.repository == 'open-mpi/ompi') && always()
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get rid of the && always(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but I wasn't sure

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, but I wasn't sure

I am not an expert on Github actions, but a function named always() seems to imply that it is always true, and when you've now got another condition in the expression, it feels like you don't need the always() (i.e., you only need always() if you have an expression with just a single condition). You might want to test this to be sure.

Copy link
Contributor Author

@B-a-S B-a-S Sep 5, 2023

Choose a reason for hiding this comment

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

There are examples of different behaviors with always() and without:
with always():
without always()
always() runs even when the job is canceled, but the condition is false w/o always()
So, clean job runs or doesn't

BTW it is the final version

Copy link
Member

Choose a reason for hiding this comment

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

Ok. If the always() is needed, then it is worth putting that in a comment, because that's not intuitive -- and we wouldn't want someone to remove the always() at some point in the future because they think it's not relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@B-a-S B-a-S force-pushed the main branch 5 times, most recently from 9b89348 to e9691fa Compare September 5, 2023 18:21
@jsquyres jsquyres merged commit 6aa55b8 into open-mpi:main Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants