[GitHub] [airflow] potiuk edited a comment on pull request #10651: Implement better shell conventions for breeze

[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