Skip to content

Conversation

@dimon222
Copy link
Contributor

@dimon222 dimon222 commented Apr 13, 2022

I don't know how it got missed after all the changes done, but the main logic still wasn't applied to have no period afterall.

In other words, the main single character change wasn't done to achieve the purpose. See changes included.
#19036

@dimon222 dimon222 requested a review from jedcunningham as a code owner April 13, 2022 14:28
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Apr 13, 2022
@potiuk
Copy link
Member

potiuk commented Apr 13, 2022

Just checking does it basically render the 4.0.0 cncf.kubernetes provider, well, useless :)?

@potiuk
Copy link
Member

potiuk commented Apr 13, 2022

(i.e. should we release 4.0.1 rather quickly ?)

@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 Apr 13, 2022
@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.

@potiuk
Copy link
Member

potiuk commented Apr 14, 2022

@dimon222 - what are the consequences of this ?

@potiuk
Copy link
Member

potiuk commented Apr 20, 2022

@dimon222 - I'd love to merge that one, but would like to know the consequence ogf "period" - I have not analyzed it but is this a breaking change/fix ? What are the consequences?

@dimon222
Copy link
Contributor Author

@potiuk
The initial root cause why I care is described there:
#16611
tl;dr hostname self-resolution problems since hostname of container looks like 2nd level DNS name.

In general, it shouldn't break much because returning back to hyphen is how it was before Airflow 2.0 upgrade that introduced it. Since the switch to hyphen happened in 95% of places of #19036 , I suspect we all agree that its intended to have hyphen. The last change required to make it happen is presented in this PR.

@potiuk potiuk merged commit 70eede5 into apache:main Apr 20, 2022
@potiuk
Copy link
Member

potiuk commented Apr 20, 2022

Ok. Cool. It's a bugfix then :)

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 (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants