Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate environment variable calculation in ShellParams in Breeze #35801

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Nov 22, 2023

Historically (when moved from Bash) the environment variables set for docker or docker-compose commands were set in a few places - some were set directly in the code, some were retrieved from shell params and eventually some were set from default values or hardcoded constants. This all happened in various models and it was scattered around the code and it was difficult to grasp what was going on.

This PR consolidates it so that all variables are set in ShellParams object.

  • the attributes have been reviewed and None/"" default values were set where needed
  • the attributes were sorted
  • calculation of dynamic properties was moved to ShellParams
  • missing properties were added to ShellParams, so that all variables have corresponding properties
  • the get_env_variables_for_docker_commands is now a method in ShellParams object, mapping is done explicitly from self.ATTRIBUTE and default values if not set are set in this single place if not set in case the variables are are retrieved from elsewhere
  • we use ShellParams in all places where we execute docker commands (we used BuildCiParams) sometimes needlessly
  • tests are added to cover the "attribute" + "incoming env var" to the env vars passed to Docker Compose/Docker

Most importantly the docker and docker-compose env files are now automatically generated and git-ignored so that we only need to maintain the list of variables to pass in a single plase - in ShellParams env_variables_for_docker_commands method.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Nov 22, 2023

This is a small follow-up after #35768 - this time not about --build-arg generation by breeze but about env variables passed to docker-compose/docker. The way it was done was quite a bit complex - and coming from the previous Bash implementation. This is I think the last remnant of the old Bash approach in Breeze's Python code :). We still have a few scripts in the container, but I think that one was the last thing that resembled the Bash version of Breeze.

cc: @Bowrna @edithturn :D

@potiuk
Copy link
Member Author

potiuk commented Nov 22, 2023

BTW, I am going to separate out small part of it - fixes to pre-commit attempting to upgrade itself in a few pre-commits (which slowed them down and made it impossible to run them in the plain without network)

@potiuk
Copy link
Member Author

potiuk commented Nov 22, 2023

There you go : #35802

@potiuk potiuk force-pushed the refactor-docker-env-preparation branch from 810ec54 to 589767c Compare November 22, 2023 17:07
@potiuk
Copy link
Member Author

potiuk commented Nov 22, 2023

BTW. This one will make it SO MUCH EASIER to understand and add new variables if we need to :)

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@potiuk
Copy link
Member Author

potiuk commented Nov 22, 2023

Some celery URL passing problem - will take a look tomorrow :)

@potiuk potiuk force-pushed the refactor-docker-env-preparation branch from 25cb870 to d0f017b Compare November 23, 2023 01:03
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@potiuk potiuk force-pushed the refactor-docker-env-preparation branch from d0f017b to 39f1e93 Compare November 23, 2023 09:18
@potiuk
Copy link
Member Author

potiuk commented Nov 23, 2023

Actually - I made it even better (see the latest version). I also figured how to do it so that we do not have to keep the separate lists of variables to pass in neither docker-compose base.yaml nor in docker.env - we are now generating the files with the list of variables for both docker and docker-compose.

After this change there is only one place where you need to add env variable to pass to docker (ShellParamss env_variables_for_docker_commamds - which will make it very straightforward on how to extend the functionality of breeze passing variable to container.

Copy link
Contributor

@utkarsharma2 utkarsharma2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. 👍

@potiuk potiuk force-pushed the refactor-docker-env-preparation branch from 39f1e93 to e9d99d8 Compare November 23, 2023 11:51
@potiuk potiuk requested a review from o-nikolas as a code owner November 23, 2023 13:31
Historically (when moved from Bash) the environment variables set for
docker or docker-compose commands were set in a few places - some were
set directly in the code, some were retrieved from shell params and
eventually some were set from default values or hardcoded constants.
This all happened in various models and it was scattered around the code
and it was difficult to grasp what was going on.

This PR consolidates it so that all variables are set in ShellParams
object.

* the attributes have been reviewed and None/"" default values were
  set where needed
* the attributes were sorted
* calculation of dynamic properties was moved to ShellParams
* missing properties were added to ShellParams, so that all variables
  have corresponding properties
* the get_env_variables_for_docker_commands is now a method in
  ShellParams object, mapping is done explicitly from self.ATTRIBUTE
  and default values if not set are set in this single place if not
  set in case the variables are are retrieved from elsewhere
* we use ShellParams in all places where we execute docker commands
  (we used BuildCiParams) sometimes needlessly
* tests are added to cover the "attribute" + "incoming env var"
  to the env vars passed to Docker Compose/Docker

Most importantly the docker and docker-compose env files are now automatically
generated and git-ignored so that we only need to maintain the list of
variables to pass in a single plase - in ShellParams
`env_variables_for_docker_commands` method.
@potiuk potiuk force-pushed the refactor-docker-env-preparation branch from 8568739 to b1691d3 Compare November 23, 2023 14:09
@potiuk potiuk merged commit 910c95e into apache:main Nov 23, 2023
72 checks passed
@potiuk potiuk deleted the refactor-docker-env-preparation branch November 23, 2023 15:08
ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
…#35801)

Historically (when moved from Bash) the environment variables set for
docker or docker-compose commands were set in a few places - some were
set directly in the code, some were retrieved from shell params and
eventually some were set from default values or hardcoded constants.
This all happened in various models and it was scattered around the code
and it was difficult to grasp what was going on.

This PR consolidates it so that all variables are set in ShellParams
object.

* the attributes have been reviewed and None/"" default values were
  set where needed
* the attributes were sorted
* calculation of dynamic properties was moved to ShellParams
* missing properties were added to ShellParams, so that all variables
  have corresponding properties
* the get_env_variables_for_docker_commands is now a method in
  ShellParams object, mapping is done explicitly from self.ATTRIBUTE
  and default values if not set are set in this single place if not
  set in case the variables are are retrieved from elsewhere
* we use ShellParams in all places where we execute docker commands
  (we used BuildCiParams) sometimes needlessly
* tests are added to cover the "attribute" + "incoming env var"
  to the env vars passed to Docker Compose/Docker

Most importantly the docker and docker-compose env files are now automatically
generated and git-ignored so that we only need to maintain the list of
variables to pass in a single plase - in ShellParams
`env_variables_for_docker_commands` method.
potiuk added a commit to potiuk/airflow that referenced this pull request Nov 27, 2023
Some of our pre-commits were using code from Breeze in rather complex
way - by inserting PYTHONPATH and importing code from there. That was
complex and brittle and with recent changes of ShelParam apache#35801 those
precommits required more and more dependencies to be added to their
pre-commit virtualenvs.

The reason that it was done this way was the assumption that someone
might want to run pre-commits locally without having breeze installed,
but this assumption and use case is rather unlikely, becasue breeze
becomes more and more useful and used so we can safely assume that
anyone who wants to do pre-commits will also have breeze installed and
on path. And anyway to run those pre-commits you need to have breeze
CI image pulled and built, so you should generally have breeze to run
them.

This PR finishes the series of PRs implementing the refactor and
completes the refactor.
potiuk added a commit that referenced this pull request Nov 27, 2023
#35830)

Some of our pre-commits were using code from Breeze in rather complex
way - by inserting PYTHONPATH and importing code from there. That was
complex and brittle and with recent changes of ShelParam #35801 those
precommits required more and more dependencies to be added to their
pre-commit virtualenvs.

The reason that it was done this way was the assumption that someone
might want to run pre-commits locally without having breeze installed,
but this assumption and use case is rather unlikely, becasue breeze
becomes more and more useful and used so we can safely assume that
anyone who wants to do pre-commits will also have breeze installed and
on path. And anyway to run those pre-commits you need to have breeze
CI image pulled and built, so you should generally have breeze to run
them.

This PR finishes the series of PRs implementing the refactor and
completes the refactor.
potiuk added a commit that referenced this pull request Dec 15, 2023
#35830)

Some of our pre-commits were using code from Breeze in rather complex
way - by inserting PYTHONPATH and importing code from there. That was
complex and brittle and with recent changes of ShelParam #35801 those
precommits required more and more dependencies to be added to their
pre-commit virtualenvs.

The reason that it was done this way was the assumption that someone
might want to run pre-commits locally without having breeze installed,
but this assumption and use case is rather unlikely, becasue breeze
becomes more and more useful and used so we can safely assume that
anyone who wants to do pre-commits will also have breeze installed and
on path. And anyway to run those pre-commits you need to have breeze
CI image pulled and built, so you should generally have breeze to run
them.

This PR finishes the series of PRs implementing the refactor and
completes the refactor.

(cherry picked from commit 8a67e15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants