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/08/30 19:39:47 UTC
[GitHub] [airflow] potiuk opened a new pull request #10651: Implement better shell conventions for breeze
potiuk opened a new pull request #10651:
URL: https://github.com/apache/airflow/pull/10651
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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).
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683495471
This is the first of a series of changes that brigs us much closer to a "good" convention for the bash scripts we have (mainly CI and Breeze). It's been asked several times in the past by @ashb and @mik-laj and they added their code following more of the ShellGuide from Google https://google.github.io/styleguide/shellguide.html (which is a very good shell convention) and I started to follow it rather rigorously.
This is the first - rather big - change that makes it fully consistent on how we use shell variables/constants in Breeze and surrounding scripts. It mainly focuses on Breeze's script, but while implementing it, I removed a number of duplications and inconsistent use of those variables elsewhere. Now there should not be any breeze-related environment variables that have inconistent behaviour.
The changes I introduced:
* constants and exported variables are CAPITALIZED, where local/temporary variables are lowercase
* following the shell guide, once all the variables are set to their final values (either from exported variables, calculation or `--switches` ) I have a single function that makes all the variables read-only. That helped to clean-up a lot of places where same functions was called several times, or where variables were defined in a few places. Now the behavior should be rather consistent and we should easily catch some duplications
* I reorganized initialization - simplified a few places where initialization was overlapping. It should be much more straightforward and clean now
* I marked a number of internal variables as "local" - this is helpful in accidental variables overwriting and keeping stuff localized
* I tested it thoroughly - both on Linux and Mac, so I do not expect many problems.
I know it's a huge change, but with this kind of changes, it has to be done as one operation (and there are not many people changing the bash scripts to be affected).
Once we merge that - we have the issue #10576 where I have at least @OmairK expressing interested in cleaning the scripts according to the shell guides, but i think merging this one first should help quite a bit to implement other changes.
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683495471
This is the first of a series of changes that brigs us much closer to a "good" convention for the bash scripts we have (mainly CI and Breeze). It's been asked several times in the past by @ashb and @mik-laj and they added their code following more of the ShellGuide from Google https://google.github.io/styleguide/shellguide.html (which is a very good shell convention) and I started to follow it rather rigorously.
This is the first - rather big - change that makes it fully consistent on how we use shell variables/constants in Breeze and surrounding scripts. It mainly focuses on Breeze's script, but while implementing it, I removed a number of duplications and inconsistent use of those variables elsewhere. Now there should not be any breeze-related environment variables that have inconistent behaviour.
The changes I introduced:
* constants and exported variables are CAPITALIZED, where local/temporary variables are lowercase
* following the shell guide, once all the variables are set to their final values (either from exported variables, calculation or `--switches` ) I have a single function that makes all the variables read-only. That helped to clean-up a lot of places where same functions was called several times, or where variables were defined in a few places. Now the behavior should be rather consistent and we should easily catch some duplications
* setting the variables as read-only also helped to clean-up the "ifs" where I often had ":=}" in variables and `!= ""` or `== ""` I replaced it with ``=}`` and tests with `-n` and `-z` - also following the shell guide (readonly helped to detect and clean all such cases). This also should be much more robust in the future.
* I reorganized initialization - simplified a few places where initialization was overlapping. It should be much more straightforward and clean now
* I marked a number of internal variables as "local" - this is helpful in accidental variables overwriting and keeping stuff localized
* I tested it thoroughly - both on Linux and Mac, so I do not expect many problems.
I know it's a huge change, but with this kind of changes, it has to be done as one operation (and there are not many people changing the bash scripts to be affected).
Once we merge that - we have the issue #10576 where I have at least @OmairK expressing interested in cleaning the scripts according to the shell guides, but i think merging this one first should help quite a bit to implement other changes.
----------------------------------------------------------------
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 a change in pull request #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#discussion_r480018616
##########
File path: scripts/in_container/_in_container_utils.sh
##########
@@ -16,6 +16,27 @@
# specific language governing permissions and limitations
# under the License.
+#######################################################################################################
+#
+# Adds trap to the traps already set.
+#
+# Arguments:
+# trap to set
+# .... list of signals to handle
+#######################################################################################################
+function add_trap() {
+ trap="${1}"
+ shift
+ for signal in "${@}"
+ do
+ # adding trap to exiting trap
+ local handlers
+ handlers="$( trap -p "${signal}" | cut -f2 -d \' )"
+ # shellcheck disable=SC2064
+ trap "${handlers}${handlers:+;}${trap}" "${signal}"
+ done
+}
+
Review comment:
Not really. The problem is that there are no common parts between the "in_container" and "out_container" stuff. We are even excluding the non-in-container scripts when we prepare the images, to make sure that what we run outside of the container is not run inside (by default). I think it's important to keep that separation. This is a really small piece of code that does not really require maintenance - kind of a "util" class (and a nice one to have at that - since we already use traps in several places and traps - unfortunately - in shell scripts are simply replacing previous traps rather than add to them.
----------------------------------------------------------------
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] feluelle commented on a change in pull request #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#discussion_r479947077
##########
File path: breeze-complete
##########
@@ -28,6 +28,25 @@ _BREEZE_ALLOWED_MYSQL_VERSIONS="5.7 8"
_BREEZE_ALLOWED_POSTGRES_VERSIONS="9.6 10"
_BREEZE_ALLOWED_KIND_OPERATIONS="start stop restart status deploy test shell"
+# Default values for the flags used
+
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_BACKEND="sqlite"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_KUBERNETES_MODE="image"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_KUBERNETES_VERSION="v1.18.6"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_KIND_VERSION="v0.8.0"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_HELM_VERSION="v3.2.4"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_POSTGRES_VERSION="9.6"
+# shellcheck disable=SC2034
+_BREEZE_DEFAULT_POSTGRES_VERSION="9.6"
Review comment:
```suggestion
```
This is a dupicate (see 2 lines above).
##########
File path: scripts/in_container/_in_container_utils.sh
##########
@@ -16,6 +16,27 @@
# specific language governing permissions and limitations
# under the License.
+#######################################################################################################
+#
+# Adds trap to the traps already set.
+#
+# Arguments:
+# trap to set
+# .... list of signals to handle
+#######################################################################################################
+function add_trap() {
+ trap="${1}"
+ shift
+ for signal in "${@}"
+ do
+ # adding trap to exiting trap
+ local handlers
+ handlers="$( trap -p "${signal}" | cut -f2 -d \' )"
+ # shellcheck disable=SC2064
+ trap "${handlers}${handlers:+;}${trap}" "${signal}"
+ done
+}
+
Review comment:
You need to duplicate the code from `_traps.sh`? Is there no better way?
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683495471
This is the first of a series of changes that brigs us much closer to a "good" convention for the bash scripts we have (mainly CI and Breeze). It's been asked several times in the past by @ashb and @mik-laj and they added their code following more of the ShellGuide from Google https://google.github.io/styleguide/shellguide.html (which is a very good shell convention) and I started to follow it rather rigorously.
This is the first - rather big - change that makes it fully consistent on how we use shell variables/constants in Breeze and surrounding scripts. It mainly focuses on Breeze's script, but while implementing it, I removed a number of duplications and inconsistent use of those variables elsewhere. Now there should not be any breeze-related environment variables that have inconistent behaviour.
The changes I introduced:
* constants and exported variables are CAPITALIZED, where local/temporary variables are lowercase
* following the shell guide, once all the variables are set to their final values (either from exported variables, calculation or `--switches` ) I have a single function that makes all the variables read-only. That helped to clean-up a lot of places where same functions was called several times, or where variables were defined in a few places. Now the behavior should be rather consistent and we should easily catch some duplications
* I added function headers (following the guide) explaining arguments, variables expected, variables modified in the functions used.
* setting the variables as read-only also helped to clean-up the "ifs" where I often had ":=}" in variables and `!= ""` or `== ""` I replaced it with ``=}`` and tests with `-n` and `-z` - also following the shell guide (readonly helped to detect and clean all such cases). This also should be much more robust in the future.
* I reorganized initialization - simplified a few places where initialization was overlapping. It should be much more straightforward and clean now
* I marked a number of internal variables as "local" - this is helpful in accidental variables overwriting and keeping stuff localized
* I tested it thoroughly - both on Linux and Mac, so I do not expect many problems.
I know it's a huge change, but with this kind of changes, it has to be done as one operation (and there are not many people changing the bash scripts to be affected).
Once we merge that - we have the issue #10576 where I have at least @OmairK expressing interested in cleaning the scripts according to the shell guides, but i think merging this one first should help quite a bit to implement other changes.
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683495471
This is the first of a series of changes that brigs us much closer to a "good" convention for the bash scripts we have (mainly CI and Breeze). It's been asked several times in the past by @ashb and @mik-laj and they added their code following more of the ShellGuide from Google https://google.github.io/styleguide/shellguide.html (which is a very good shell convention) and I started to follow it rather rigorously.
This is the first - rather big - change that makes it fully consistent on how we use shell variables/constants in Breeze and surrounding scripts. It mainly focuses on Breeze's script, but while implementing it, I removed a number of duplications and inconsistent use of those variables elsewhere. Now there should not be any breeze-related environment variables that have inconistent behaviour.
The changes I introduced:
* constants and exported variables are CAPITALIZED, where local/temporary variables are lowercase
* following the shell guide, once all the variables are set to their final values (either from exported variables, calculation or `--switches` I have a single function that makes all the variables read-only. That helped to clean-up a lot of places where same functions was called several times, or where variables were defined in a few places. Now the behavior should be rather consistent and we should easily catch some duplications
* I reorganized initialization - simplified a few places where initialization was overlapping. It should be much more straightforward and clean now
* I marked a number of internal variables as "local" - this is helpful in accidental variables overwriting and keeping stuff localized
* I tested it thoroughly - both on Linux and Mac, so I do not expect many problems.
I know it's a huge change, but with this kind of changes, it has to be done as one operation (and there are not many people changing the bash scripts to be affected).
Once we merge that - we have the issue #10576 where I have at least @OmairK expressing interested in cleaning the scripts according to the shell guides, but i think merging this one first should help quite a bit to implement other changes.
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683703494
@mik-laj @feluelle i also spearated out some small unrelated changes to a separate PR and added more detailed explanation in the commit message.
----------------------------------------------------------------
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 #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10651:
URL: https://github.com/apache/airflow/pull/10651#issuecomment-683702514
> Should we also add a linter that checks those style rules?
I think it's super difficult. We already had before `shellcheck` (which is also recommended by the Google Shell Guide BTW). And it catches (already did) a number of cases, but I am not sure there is a more comprehensive linter to check some of those.
* it's rather difficult to say what is a "constant" and what is a "variable". In our case I marked as "constant" (i.e. CAPITALIZED) all the variables that result from choosing the appropriate `--switches` in Breeze. But they cannot be "defined" as constants, because they have to be set as defaults and only after all parsing of parameters is done we can mark them as "readonly" (and we do).
* There are literally a few of those that are still "exported" but not constant (because they can change mid-way of pre-commits for example). `FORCE_ANSWERS_TO_QUESTIONS` is a good example. You can set it with switches (`--force-yes`) but even if you do not do it, in pre-commits you want to set it to `yes` after the build step successfully builds the image - so that you can run several pre-commits in a row and build the image only once - similarly `SKIP_BUILD_IMAGES` must be exported but it is not constant (but the fact that it is exported, still makes it capitalized)
* the rules about `:-`, `-z`, `-n` are difficult to enforce. However those are enforced dynamically (not statically) with the `readonly` status. When you try to do `:=` with such variable you will get an error. So some of those rules are now verified dynamically.
----------------------------------------------------------------
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 merged pull request #10651: Implement better shell conventions for breeze
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10651:
URL: https://github.com/apache/airflow/pull/10651
----------------------------------------------------------------
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