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.

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

Saving the VCS Checkout dir path to an environment variable #564

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Saving the VCS Checkout dir path to an environment variable #564

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55355cd
feat(task_context): add task name to subs implicitly
ahal Aug 15, 2024
9afe282
fix: save vcs checkout dir to env var
gshiva53 Aug 17, 2024
0137d23
fix: save vcs checkout dir path if exists
gshiva53 Aug 19, 2024
38a9c42
fix: save if primary repo path exists
gshiva53 Aug 19, 2024
3d0ae4c
fix: save vcs path dir with error checking
gshiva53 Aug 19, 2024
5dec871
fix: save primary repo path on first iteration
gshiva53 Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/taskgraph/transforms/run/run_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ def common_setup(config, task, taskdesc, command):
for repo_config in repo_configs.values():
checkout_path = path.join(vcs_path, repo_config.path)
command.append(f"--{repo_config.prefix}-checkout={checkout_path}")
taskdesc["worker"]["env"]["VCS_CHECKOUT_DIR_PATH"] = checkout_path
Copy link
Collaborator

@ahal ahal Aug 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the patch! This is close, but there are a couple things to fix:

  1. Can we call the env PRIMARY_REPO_PATH instead?
  2. If there are multiple entries in repo_configs, later entries will overwrite the earlier ones. Instead, I think we should only save the first entry in the repo_configs. So we should only save the env if it doesn't exist yet.
  3. It looks like this causes the tests to fail. You'll need to fix that up before this can land. This should be a matter of updating the expectations, though it looks like the tests also set repo_config.path to an empty string.. it would be nice to change them to use a proper value here as well.

Thanks for your interest and please don't hesitate to reach out if you need help with any of this.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your message, I really appreciate the feedback.

  1. Changed the path var name.
  2. Added a check to see if a checkout dir path already exists, it will only save if the dir path does not exist.
  3. I am a bit unsure of how to change the repo_config.path to some meaningful value. I looked at the failing tests and it seems like it's primarily because the task is unable to run. So, multiple tests fail for the same reason it seems.

I would love to make any further changes if required. Thanks again for your feedback.


if run["sparse-profile"]:
command.append(
Expand Down