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

Fix Command Dispatcher CI Bug #765

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Fix Command Dispatcher CI Bug #765

merged 1 commit into from
Oct 24, 2023

Conversation

AdnaneKhan
Copy link
Contributor

Pull Request Description

This pull request updates the command dispatcher workflow to perform a check for an actor within an Array instead of a String. String contains a substring check so that substring usernames would pass the check.

See: https://docs.github.com/en/actions/learn-github-actions/expressions#contains

Acceptance Test

N/A

Questions

  • Does this pull request break backward compatibility?

  • Does this pull request require other pull requests to be merged first?

    • Yes, please see #...
    • No
  • Does this require an update of the documentation?

    • Yes, please see [provide details here]
    • No

@steve-todorov
Copy link
Collaborator

steve-todorov commented Oct 24, 2023

Hey @AdnaneKhan,

Github Actions has a hidden "feature" (or rather bug) which we appear to be using:

      - name: Test
        if: |
          contains('["carlspring", "steve-todorov"]', 'steve-todorov')
        run: |
          echo "Runs."

      - name: Does not run
        if: |
          contains('["carlspring", "steve-todorov"]', 'test')
        run: |
          echo "Does not run."

      - name: Test
        if: |
          contains(fromJSON('["carlspring", "steve-todorov"]'), 'steve-todorov')
        run: |
          echo "Runs."

      - name: Does not run
        if: |
          contains(fromJSON('["carlspring", "steve-todorov"]'), 'test')
        run: |
          echo "Does not run."

However I believe it is better to use fromJSON() as GH might change this.
Thank you very much for pointing this out!

@steve-todorov steve-todorov merged commit b9cef12 into carlspring:master Oct 24, 2023
@github-actions github-actions bot mentioned this pull request Oct 24, 2023
@AdnaneKhan
Copy link
Contributor Author

Hey @AdnaneKhan,

Github Actions has a hidden "feature" (or rather bug) which we appear to be using:

      - name: Test
        if: |
          contains('["carlspring", "steve-todorov"]', 'steve-todorov')
        run: |
          echo "Runs."

      - name: Does not run
        if: |
          contains('["carlspring", "steve-todorov"]', 'test')
        run: |
          echo "Does not run."

      - name: Test
        if: |
          contains(fromJSON('["carlspring", "steve-todorov"]'), 'steve-todorov')
        run: |
          echo "Runs."

      - name: Does not run
        if: |
          contains(fromJSON('["carlspring", "steve-todorov"]'), 'test')
        run: |
          echo "Does not run."

However I believe it is better to use fromJSON() as GH might change this. Thank you very much for pointing this out!

Ah, if you replaced ‘test’ with ‘arlspring’, then the old one would have worked (hence the risk - someone could register a username that was a sub string to bypass). But it’s all good now!

@steve-todorov
Copy link
Collaborator

Ahh yeah that would have been a problem indeed. Thank again for pointing it out!

@AdnaneKhan AdnaneKhan deleted the AdnaneKhan-patch-1 branch October 24, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants