You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/07/03 18:00:48 UTC
[GitHub] [airflow] potiuk opened a new pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
potiuk opened a new pull request #9647:
URL: https://github.com/apache/airflow/pull/9647
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653649541
> Please do. I think it's the "eat cake and have it too" case. We can fulfill all your expectations (separate releases, issues for users in separate repos) while keeping much lower complexity of the development process.
Yup definitely :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653644916
On the flip side changes in the Helm Chart should not affect Airflow's CI. In this case, the default of disabling API should just be a change in Airflow and should not be a change in the Helm chart version until a new Airflow version is released.
> Having a dedicated config is much nicer solution. And if we were to do this for split repos we would have to implement "workaround" first, "good solution" in a chart repo, release the repo and finally implement a "good" solution. I am sure it is not worth it and I love how kubernetes team solved it.
We already have a dedicated config `default_test.cfg` (https://github.com/apache/airflow/blob/master/airflow/config_templates/default_test.cfg) we should be using that for tests.
I don't see a need of "workaround" here?
Btw I am not against the Kubernetes way, I will look into the details and let you'll know on the thread. But as of now I am still on the "separate repo" side
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653643245
> Well you could just set up Environment variable, here
>
> https://github.com/apache/airflow/blob/master/chart/values.yaml#L94-L98
>
> So theory and in-practise here are not different
But this could be much more complex change needed to make this works - this is really one liner that shows how simple it is now to fix those kind of problems and how complex it could be if we split.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653642786
> I think it should still work. In theory, the chart should allow overriding any runtime Airflow configs. In this case we could have used Environment Variable to set config. If we are not able to do we would anyway hit that limitation
In theory, theory and practice are the same. In practice, they are not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653639691
Side note
@kaxil @dimberman @ashb @turbaszek @mik-laj - this is an example of where splitting to separate repo would have been problematic.
This change should have been committed together with #9611 and it would have been rather complex to test if the helm chart and kubernetes were separate repos. It would not work with the scheme proposed by @kaxil where we only use "released" versions of helm chart for Airflow tests and the other way round because in this case changing the backend auth to "deny_all" had to be coupled with adding new configuration value in the helm chart.
It could work with the submodule scheme I proposed but as discussed with @dimberman yesterday - just coordinating this simple change across two PRs done in two different repos would have been a much more complex task.
And this is just one small change ...
I really think that monorepo approach and possibly split-out to separate repos with pushing only related changes (like @ryw commented) could be a much better solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653639691
Side note
@kaxil @dimberman @ashb @turbaszek @mik-laj - this is an example of where splitting to separate repo would have been problematic.
This change should have been committed together with #9611 and it would have been rather complex to test if the `chart` and `airflow` were separate repos. It would not work with the scheme proposed by @kaxil where we only use "released" versions of helm chart for Airflow tests and the other way round because in this case changing the backend auth to "deny_all" had to be coupled with adding new configuration value in the helm chart.
It could work with the submodule scheme I proposed but as discussed with @dimberman yesterday - just coordinating this simple change across two PRs done in two different repos would have been a much more complex task.
And this is just one small change ...
I really think that monorepo approach and possibly split-out to separate repos with pushing only related changes (like @ryw commented) could be a much better solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653643945
Having a dedicated config is much nicer solution. And if we were to do this for split repos we would have to implement "workaround" first, "good solution" in a chart repo, release the repo and finally implement a "good" solution. I am sure it is not worth it and I love how kubernetes team solved it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653646797
> On the flip side changes in the Helm Chart should not affect Airflow's CI. In this case, the default of disabling API should just be a change in Airflow and should not be a change in the Helm chart version until a new Airflow version is released.
*Should*. This is a nice idea, but until we are really stable and have all the details worked out for helm chart and dockerfile there will be hiden couplings - even beyond that. IMHO this is the same fallacy as with micro-services hype. With Micro-services in theory you have decoupled services that can be developed in isolation but in fact a lot of micro-services have hidden couplings and what you gain with separation, you loose on coordination. A lot of teams (especially those not huge ones) withdraw from micro-services approach (I'd say hype) recently, precisely because it is not full-filing the promises (i.e. for smaller teams costs of coordination are far bigger than benefits of isolation). It's never 0-1, it's always cost-gain game.
I believe we are still far to small (both code-wise and people-wise) and the "chart" and "dockerfile" are not big enough on it's own to get enough isolation gains to cover the separation cost.
> Btw I am not against the Kubernetes way, I will look into the details and let you'll know on the thread. But as of now I am still on the "separate repo" side
Please do. I think it's the "eat cake and have it too" case. We can fulfill all your expectations (separate releases, issues for users in separate repos) while keeping much lower complexity of the development process.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653642379
> Side note
>
> @kaxil @dimberman @ashb @turbaszek @mik-laj - this is an example of where splitting to separate repo would have been problematic.
>
> This change should have been committed together with #9611 and it would have been rather complex to test if the `chart` and `airflow` were separate repos. It would not work with the scheme proposed by @kaxil where we only use "released" versions of helm chart for Airflow tests and the other way round because in this case changing the backend auth to "deny_all" had to be coupled with adding new configuration value in the helm chart.
>
> It could work with the submodule scheme I proposed but as discussed with @dimberman yesterday - just coordinating this simple change across two PRs done in two different repos would have been a much more complex task.
>
> And this is just one small change ...
>
> I really think that monorepo approach and possibly split-out to separate repos with pushing only related changes (like @ryw commented) could be a much better solution.
I think it should still work. If it doesn't the chart is not robust enough. In theory, the chart should allow overriding any runtime Airflow configs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #9647:
URL: https://github.com/apache/airflow/pull/9647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653643433
But why not use Environment Variables to set configs. That's what we want to allow users to do too with the Chart, they would like to override any config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653642379
> Side note
>
> @kaxil @dimberman @ashb @turbaszek @mik-laj - this is an example of where splitting to separate repo would have been problematic.
>
> This change should have been committed together with #9611 and it would have been rather complex to test if the `chart` and `airflow` were separate repos. It would not work with the scheme proposed by @kaxil where we only use "released" versions of helm chart for Airflow tests and the other way round because in this case changing the backend auth to "deny_all" had to be coupled with adding new configuration value in the helm chart.
>
> It could work with the submodule scheme I proposed but as discussed with @dimberman yesterday - just coordinating this simple change across two PRs done in two different repos would have been a much more complex task.
>
> And this is just one small change ...
>
> I really think that monorepo approach and possibly split-out to separate repos with pushing only related changes (like @ryw commented) could be a much better solution.
I think it should still work. In theory, the chart should allow overriding any runtime Airflow configs. In this case we could have used Environment Variable to set config. If we are not able to do we would anyway hit that limitation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653669532
Monorepo FTW.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653642379
> Side note
>
> @kaxil @dimberman @ashb @turbaszek @mik-laj - this is an example of where splitting to separate repo would have been problematic.
>
> This change should have been committed together with #9611 and it would have been rather complex to test if the `chart` and `airflow` were separate repos. It would not work with the scheme proposed by @kaxil where we only use "released" versions of helm chart for Airflow tests and the other way round because in this case changing the backend auth to "deny_all" had to be coupled with adding new configuration value in the helm chart.
>
> It could work with the submodule scheme I proposed but as discussed with @dimberman yesterday - just coordinating this simple change across two PRs done in two different repos would have been a much more complex task.
>
> And this is just one small change ...
>
> I really think that monorepo approach and possibly split-out to separate repos with pushing only related changes (like @ryw commented) could be a much better solution.
I think it should still work. In theory, the chart should allow overriding any runtime Airflow configs. In this case we could have used Environment Variable to set config. If we are not able to do we would anyway hit that limitation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653649541
> Please do. I think it's the "eat cake and have it too" case. We can fulfill all your expectations (separate releases, issues for users in separate repos) while keeping much lower complexity of the development process.
Yup ignore my last comment, I will definitely give the k8s approach a good look and get back on the discussion :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653648597
> > On the flip side changes in the Helm Chart should not affect Airflow's CI. In this case, the default of disabling API should just be a change in Airflow and should not be a change in the Helm chart version until a new Airflow version is released.
>
> _Should_. This is a nice idea, but until we are really stable and have all the details worked out for helm chart and dockerfile there will be hiden couplings - even beyond that. IMHO this is the same fallacy as with micro-services hype. With Micro-services in theory you have decoupled services that can be developed in isolation but in fact a lot of micro-services have hidden couplings and what you gain with separation, you loose on coordination. A lot of teams (especially those not huge ones) withdraw from micro-services approach (I'd say hype) recently, precisely because it is not full-filing the promises (i.e. for smaller teams costs of coordination are far bigger than benefits of isolation). It's never 0-1, it's always cost-gain game.
>
> I believe we are still far to small (both code-wise and people-wise) and the "chart" and "dockerfile" are not big enough on it's own to get enough isolation gains to cover the separation cost.
>
> > Btw I am not against the Kubernetes way, I will look into the details and let you'll know on the thread. But as of now I am still on the "separate repo" side
>
> Please do. I think it's the "eat cake and have it too" case. We can fulfill all your expectations (separate releases, issues for users in separate repos) while keeping much lower complexity of the development process.
I would say we should not use the Helm Chart for test until it gets stable then. WDYT?
Let's work on getting the Helm Chart to the ideal stable before using it for tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653646797
> On the flip side changes in the Helm Chart should not affect Airflow's CI. In this case, the default of disabling API should just be a change in Airflow and should not be a change in the Helm chart version until a new Airflow version is released.
*Should*. This is a nice idea, but until we are really stable and have all the details worked out for helm chart and dockerfile there will be hiden couplings - even beyond that. IMHO this is the same fallacy as with micro-services hype. With Micro-services in theory you have decoupled services that can be developed in isolation but in fact a lot of micro-services have hidden couplings and what you gain with separation, you loose on coordination. A lot of teams (especially those not huge ones) withdraw from micro-services approach (I'd say hype) recently, precisely because it is not full-filing the promises (i.e. for smaller teams costs of coordination are far bigger than benefits of isolation). It's never 0-1, it's always cost-gain game.
I believe we are still far to small (both code-wise and people-wise) and the "chart" and "dockerfile" are not big enough on it's own to get enough isolation benefits to cover the separation gain.
> Btw I am not against the Kubernetes way, I will look into the details and let you'll know on the thread. But as of now I am still on the "separate repo" side
Please do. I think it's the "eat cake and have it too" case. We can fulfill all your expectations (separate releases, issues for users in separate repos) while keeping much lower complexity of the development process.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9647:
URL: https://github.com/apache/airflow/pull/9647#issuecomment-653642913
Well you could just set up Environment variable, here
https://github.com/apache/airflow/blob/master/chart/values.yaml#L94-L98
So theory and in-practise here are not different
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org