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

Restore how the dask worker node group is selected by default #1577

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

iameskild
Copy link
Member

Reference Issues or PRs

What does this implement/fix?

  • I thought I tested this PR but it appears I didn't. That PR does fix another bug but accidentally breaks how the dask worker node group selection works by default.

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Any other comments?

@iameskild iameskild mentioned this pull request Dec 1, 2022
11 tasks
@iameskild iameskild added type: bug 🐛 Something isn't working area: integration/Dask Issues related to Dask on QHub labels Dec 1, 2022
@aktech
Copy link
Member

aktech commented Dec 1, 2022

It's quite surprising that Kubernetes tests and Provider Tests are skipped. 🤔

@viniciusdc
Copy link
Contributor

It's quite surprising that Kubernetes tests and Provider Tests are skipped. thinking

Could it be this line?

if: github.event.pull_request.head.repo.name == github.repository || github.event_name != 'pull_request'

Maybe with a push to this branch, the action will be triggered... this is another good example on why we need to refactor the CI/CD, at least the triggering logic

@aktech
Copy link
Member

aktech commented Dec 1, 2022

We can add this job in the PR at the start to find out:

  precheck:
    name: "Check for following jobs"
    runs-on: ubuntu-latest
    steps:
      - name: Check
        run: |
          echo "HEAD Repository: ${{ github.event.pull_request.head.repo.name }}"
          echo "github.repository: ${{ github.repository}}"
          echo "github.event_name: ${{ github.event_name }}"
          echo "Should the following job run:"
          echo ${{ github.event.pull_request.head.repo.name == github.repository || github.event_name != 'pull_request' }}

Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

LGTM

@iameskild
Copy link
Member Author

Thanks @viniciusdc @aktech! Did you want to test that section in this PR?

@aktech
Copy link
Member

aktech commented Dec 1, 2022

We can but I don't want to block this PR, since it's approved, let's get this one in, we can test in a separate PR.

@iameskild iameskild merged commit 5ff666c into release/2022.11.1 Dec 1, 2022
@iameskild iameskild deleted the 20221130eae branch December 1, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: integration/Dask Issues related to Dask on QHub type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants