Skip to content

Mark Airflow directory in container as safe for git commands #29386

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.

Sign up for GitHub

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

Jump to bottom
Merged
potiuk merged 1 commit into from
Feb 6, 2023
Merged

Mark Airflow directory in container as safe for git commands #29386

potiuk merged 1 commit into from
Feb 6, 2023

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Feb 6, 2023

There is a new setting/version of git in GitHub Actions that started checking the ownership of the Git repository. Since in case of the provider commands we run them inside docker image as root user (this is in order to isolate the provider package building from the main CI environment), the owner of such directory is different (runner user) than the user that runs the git command (root).

This change marks the current git directory for such commands as safe, regardles from the discrepancy.

This config is global and run inside the image, so it is safe to leave it after methods complete as containers are torn-down after completing package preparation.

This PR also improves diagnostics. Previously the git remote add output was redirected to dev null as there was no way it could fail, but this turned to be false - the output of the git remote add commnd is now also printed for diagnostics.

^ 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 potiuk requested review from Taragolis and eladkal February 6, 2023 12:15
@potiuk potiuk force-pushed the fix-error-with-dubious-ownership branch 2 times, most recently from 00ab0ab to 76eb7e0 Compare February 6, 2023 12:31
@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

Almost... Had to explicitly add "/opt/airflow"

@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

OK. Package preparation passed the place where it errored out. Build PROD images should get fixed automatically after merging.

@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

Not really seems that the new git has also another back-compat problem with retrieving git revision revision retrieval which prepares Airflow package and returns empty-hash (impact actual airflow prepareation). Need to take a look later (in 1 hour or so).

@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

(Unless someone else wants to take a look till then).

@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

BTW. Got at least clarity how we got there:

  • we upgraded to latest debian base images automatically (with scheduled run)
  • Looks like this is a problem with new git between 2.30.2 (default for previous debian/python image we had) and 2.38.1 (default in latest images).

This method (from airflow's setup.py) stopped returning git hash for airflow.

def git_version(version_: str) -> str:
    """
    Return a version to identify the state of the underlying git repo. The version will
    indicate whether the head of the current git-backed working directory is tied to a
    release tag or not : it will indicate the former with a 'release:{version}' prefix
    and the latter with a '.dev0' suffix. Following the prefix will be a sha of the current
    branch head. Finally, a "dirty" suffix is appended to indicate that uncommitted
    changes are present.

    :param str version_: Semver version
    :return: Found Airflow version in Git repo
    """
    try:
        import git

        try:
            repo = git.Repo(str(AIRFLOW_SOURCES_ROOT / ".git"))
        except (git.NoSuchPathError):
            logger.warning(".git directory not found: Cannot compute the git version")
            return ""
        except git.InvalidGitRepositoryError:
            logger.warning("Invalid .git directory not found: Cannot compute the git version")
            return ""
    except ImportError:
        logger.warning("gitpython not found: Cannot compute the git version.")
        return ""
    if repo:
        sha = repo.head.commit.hexsha
        if repo.is_dirty():
            return f".dev0+{sha}.dirty"
        # commit is clean
        return f".release:{version_}+{sha}"
    return "no_git_version"

@potiuk potiuk mentioned this pull request Feb 6, 2023
Merged
@potiuk
Mark Airflow directory in container as safe for git commands
341cbbf 
There is a new setting/version of git in GitHub Actions that started
checking the ownership of the Git repository. Since in case of the
provider commands we run them inside docker image as root user
(this is in order to isolate the provider package building from
the main CI environment), the owner of such directory is different
(runner user) than the user that runs the git command (root).

This change marks the current git directory for such commands as
safe, regardles from the discrepancy.

This config is global and run inside the image, so it is safe to
leave it after methods complete as containers are torn-down after
completing package preparation.

This PR also improves diagnostics. Previously the `git remote add`
output was redirected to dev null as there was no way it could fail,
but this turned to be false - the output of the `git remote add`
commnd is now also printed for diagnostics.
@potiuk potiuk force-pushed the fix-error-with-dubious-ownership branch from 76eb7e0 to 341cbbf Compare February 6, 2023 15:30
@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

OK. Seems that it was the same reason - git complained about different user owning the .git repo and version was not retrievable. Added the same exclusion for building airflow packages.

@potiuk
Copy link
Member Author

potiuk commented Feb 6, 2023

OK. Issue solved (there are tests failing in main due to some upgraded dependencies, but this is another issue and can be solved later.

@potiuk potiuk merged commit 2e1635a into apache:main Feb 6, 2023
@potiuk potiuk deleted the fix-error-with-dubious-ownership branch February 6, 2023 16:02
@potiuk potiuk added this to the Airflow 2.5.2 milestone Feb 8, 2023
@pierrejeambrun pierrejeambrun added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 6, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
@potiuk @pierrejeambrun
Mark Airflow directory in container as safe for git commands (#29386)
1dc9d73 
There is a new setting/version of git in GitHub Actions that started
checking the ownership of the Git repository. Since in case of the
provider commands we run them inside docker image as root user
(this is in order to isolate the provider package building from
the main CI environment), the owner of such directory is different
(runner user) than the user that runs the git command (root).

This change marks the current git directory for such commands as
safe, regardles from the discrepancy.

This config is global and run inside the image, so it is safe to
leave it after methods complete as containers are torn-down after
completing package preparation.

This PR also improves diagnostics. Previously the `git remote add`
output was redirected to dev null as there was no way it could fail,
but this turned to be false - the output of the `git remote add`
commnd is now also printed for diagnostics.

(cherry picked from commit 2e1635a)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
@potiuk @pierrejeambrun
Mark Airflow directory in container as safe for git commands (#29386)
ce5b166 
There is a new setting/version of git in GitHub Actions that started
checking the ownership of the Git repository. Since in case of the
provider commands we run them inside docker image as root user
(this is in order to isolate the provider package building from
the main CI environment), the owner of such directory is different
(runner user) than the user that runs the git command (root).

This change marks the current git directory for such commands as
safe, regardles from the discrepancy.

This config is global and run inside the image, so it is safe to
leave it after methods complete as containers are torn-down after
completing package preparation.

This PR also improves diagnostics. Previously the `git remote add`
output was redirected to dev null as there was no way it could fail,
but this turned to be false - the output of the `git remote add`
commnd is now also printed for diagnostics.

(cherry picked from commit 2e1635a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eladkal eladkal eladkal approved these changes

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@Taragolis Taragolis Awaiting requested review from Taragolis

Assignees

No one assigned

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Milestone

Airflow 2.5.2

Development

Successfully merging this pull request may close these issues.

3 participants

@potiuk @eladkal @pierrejeambrun