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

[AIRFLOW-3760] - Ensure that we pass the command to the Kubernetes Pod as a List #4575

Closed
wants to merge 1 commit into from

Conversation

PaulW
Copy link
Contributor

@PaulW PaulW commented Jan 23, 2019

Make sure you have checked all steps below.

Jira

Description

  • AIRFLOW-2888 caused worker pod creation to fail, as we are not passing the command to kubernetes as a list item.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason: This fixes an issue created with a previous pull request.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

  • Passes flake8

@PaulW PaulW changed the title Ensure that we pass the command to the Kubernetes Pod as a List [AIRFLOW-3760] - Ensure that we pass the command to the Kubernetes Pod as a List Jan 23, 2019
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This is not the right level to fix it, and splitting on space is error prone.

The correct way to fix this would be to make sure this stays as a list though all layers in think.

This also needs tests.

@timpharo
Copy link

timpharo commented Jan 24, 2019

This breaking change is affecting us too. From what I can see and have read from the description this is a contribution back for a breaking change that was made in #3740.
Looking through the original PR, the changes made that are causing issues with 'cmds' seems unrelated to the description of the original pull request and furthermore the original pull request doesn't have any tests, stating "Tests updated (but not functionally)".

As an open source project that to my knowledge is very well used now, I find it hard to believe that this kind of thing has been merged with comments outstanding such as "The airflow command is not passed as a string anymore. I need some help in doing this properly for the k8s executor most likely".

As the merge of #3740 has broken the K8 functionality, it seems that the correct action would be to revert those changes and re-implement with better testing in place.

@ashb
Copy link
Member

ashb commented Jan 24, 2019

Not disputing this is a problem, but PR#3760 is not yet included in an published release so that can't be the source of the issue.

@PaulW PaulW closed this Jan 24, 2019
@PaulW PaulW deleted the AIRFLOW-3760 branch January 24, 2019 13:04
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.

3 participants