[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9647: Fixed failing Kubernetes tests after deny_all for experimental API
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: us...@infra.apache.org
