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

Allow omission of initial_node_count if node_pools is specified #17820

Merged
merged 15 commits into from
Aug 27, 2021

Conversation

leahecole
Copy link
Contributor

@leahecole leahecole commented Aug 25, 2021

According to the API reference, the initial_node_count should not be used when you are configuring node_pools because you specify the initial_node_count within the node_pools object.

This PR adds logic allowing either initial_node_count OR node_pools and updates test cases. I also refactored the test to make it more human readable because boolean logic makes me dizzy sometimes 😄

@leahecole leahecole added the provider:google Google (including GCP) related issues label Aug 25, 2021
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Aug 25, 2021
@leahecole
Copy link
Contributor Author

Apologies - I'll fix the static checks later today. I also have a second check I think i need to add to make sure you don't have both an initial node count AND nodepools

@leahecole leahecole marked this pull request as draft August 25, 2021 22:04
@leahecole leahecole marked this pull request as ready for review August 26, 2021 22:00
@leahecole
Copy link
Contributor Author

Okay! This is ready. I got myself really tangled in some boolean logic but have since recovered 😅

@leahecole leahecole requested a review from mik-laj August 27, 2021 15:28
Copy link
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

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

LGTM. However, I think that we can safely delete this validation and rely only on the server-side validation.

PS. Project ID should be optionally, because we can obtain a project ID from credential.

@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Aug 27, 2021
@leahecole leahecole merged commit 87769db into apache:main Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants