-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Checkout entire git history in test-command #22184
Conversation
/test connector=connectors/source-pokeapi
Build PassedTest summary info:
|
Action using master: https://github.com/airbytehq/airbyte/actions/runs/4059525463 |
Airbyte Code Coverage
|
@evantahler Always hot on my heels! Youre right it didnt run with the updated config. But I just triggered a new build targeting the new workflow here: https://github.com/airbytehq/airbyte/actions/runs/4059601806/workflow So we will see! |
It still failed 🤔 |
I recently added the CWD change, which I think should be having no effect, but that could be wrong - airbyte/tools/ci_connector_ops/ci_connector_ops/utils.py Lines 15 to 17 in 9c4100b
|
/test connector=connectors/source-faker gitref=bechurch/feat-qa-engine-22127-git-history
Build FailedTest summary info:
|
/test connector=connectors/source-pokeapi gitref=bechurch/feat-qa-engine-22127-git-history
Build PassedTest summary info:
|
New build in progress https://github.com/airbytehq/airbyte/actions/runs/4069882520 Had assumed the above command would have ran the qa-engine test like expected. |
9765f22
to
fc7bebd
Compare
Running another action: https://github.com/airbytehq/airbyte/actions/runs/4071070930 Basically reverted to the original. I want to see the current working directory this is running at. |
@airbytehq/connector-operations Sorry for all the pings. This should be now ready for review! QA Checks are back! https://github.com/airbytehq/airbyte/actions/runs/4078879071 |
AIRBYTE_REPO = git.Repo(".") | ||
AIRBYTE_REPO = git.Repo(search_parent_directories=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, of course - the magic option was set to False...
Would this still work if we added back the os.chdir('../../..')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not unfortunately.
The issue here came down to the file found it self in different directories and running contexts.
So change the directory via a relative path would always break one of local
or ec2
On local
the paths were:
- current working directory: /Users/ben/Development/repos/airbyte/tools/ci_connector_ops
- file directory: /Users/ben/Development/repos/airbyte/tools/ci_connector_ops/ci_connector_ops
On the ec2
instance the paths were (taken from a workflow we ran):
- The current working directory was
/actions-runner/_work/airbyte/airbyte
- And the files path was
/actions-runner/_work/_tool/Python/3.9.16/x64/lib/python3.9/site-packages/ci_connector_ops/utils.py
So our best way seems to be relying on using the GitPython library to simply traverse up the directory structure until it finds a .git
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shall pass this repo path as an env var for a more explicit definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Thinking on that a bit. Perhaps if theres is a case where we would not be inside a git versioned directory.
@alafanechere Is there a context that might happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice debugging 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think explicit path definition via env var would make this more stable and flexible in the long run. Feel free to add it to the PR if you like this suggestion, LGTM otherwise.
AIRBYTE_REPO = git.Repo(".") | ||
AIRBYTE_REPO = git.Repo(search_parent_directories=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shall pass this repo path as an env var for a more explicit definition
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
/test connector=connectors/source-bigquery
Build PassedTest summary info:
|
Running a couple more tests before merge |
/test connector=connectors/source-sentry
Build FailedTest summary info:
|
/test connector=connectors/source-bigquery
Build PassedTest summary info:
|
What
We were seeing a failure as the git history was not available.
Fix #22127
How
I believe the issue is that we didnt have the full history of the project.
This PR should let us test that that is true as we download full history
🚨 User Impact 🚨
This could result in longer than acceptable build times
Pre-merge Checklist
Post-merge Checklist
test-command.yml
report.sh
to use theQA_CHECKS_OUTCOME
argument again