Fixing Detection of Unpinned Dependencies for DDEV Validate DEP #23584
Fixing Detection of Unpinned Dependencies for DDEV Validate DEP #23584ddog-nasirthomas wants to merge 13 commits intomasterfrom
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: efa826b | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 009ff5a2da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| agent_dependencies_file = get_agent_requirements() | ||
| annotate_errors(agent_dependencies_file, agent_errors) | ||
| ctx = click.get_current_context() | ||
| repo_core = ctx.obj.repo.name == 'core' |
There was a problem hiding this comment.
Compute repo_core from ctx.obj.repo.name misclassifies ddev -x runs inside integrations-core as non-core, because Application.set_repo(..., here=True) sets the repo name to local; this silently disables pinning and agent-sync validations that should still run for core checkouts. It can also break the legacy standalone CLI path where ctx.obj is a dict (tooling/cli.py), not an object with .repo, causing an attribute error instead of validating dependencies.
Useful? React with 👍 / 👎.
|
This PR does not modify any files shipped with the agent. To help streamline the release process, please consider adding the |
| ctx = click.get_current_context() | ||
| obj = ctx.obj | ||
| repo_choice = None | ||
|
|
||
| if hasattr(obj, 'get'): | ||
| repo_choice = obj.get('repo_choice') or obj.get('repo') | ||
| if not repo_choice and hasattr(obj, 'repo'): | ||
| repo_choice = getattr(obj.repo, 'name', None) | ||
| repo_core = repo_choice == 'core' |
There was a problem hiding this comment.
Is there a reason we can't use ctx.obj['repo_choice'] == core here?
There was a problem hiding this comment.
We technically could do that. The longer block is defensive for cases where repo_choice is never written on the legacy config object. For example, initialize_root returns immediately when check_root() is true, so it never runs the code that sets config['repo_choice']
def initialize_root(config, agent=False, core=False, extras=False, marketplace=False, here=False, **kwargs):
"""Initialize root directory based on config and options"""
if check_root():
returnIn this case, the repo name still exists when the application is started. I fallback to what the repo name is set to. code
self.__config['repo'] = self.repo.name
Let me know if my understanding is correct.
There was a problem hiding this comment.
I agree there is a valid path where config['repo_choice'] is not set, but most ddev commands use this approach with the same limitation. I think it's ok to leave it as is since check_root is rarely true and it follows the pattern of other ddev commands. What about instead making a PR to always initialize config['repo_choice'], even if check_root is true? Let me know what you think!
There was a problem hiding this comment.
Sure, I think that's a great idea. I'll create the PR for that and use ctx.obj['repo_choice'] == core for determining the repo in this PR.
There was a problem hiding this comment.
Okay I updated how repo_core is defined. Also I had to update two tests test_multiple_invalid_third_party_integrations and test_one_valid_one_invalid_integration since they called ddev validate dep twice. On the second call the repo_choice did not initalize because the global integrations root was already set after the first run, so initialize_root hit the check_root() guard and returned before it could set repo_choice on the new config.
Working on the other pr to always set repo_choice
There was a problem hiding this comment.
PR for setting repo_choice is here: #23649
There was a problem hiding this comment.
Commands are always run through ddev and the context object is the Application from ddev. Use Application.repo properties instead of update a legacy soon to be removed process. I added a comment on the other PR.
I am also unsure about whether we want to do this fix here or migrate the command driectly to ddev. We are in the middle of migrating everything at the moment. Maybe we can hold on on this and do the fix when we are not including new logic in the legacy checks dev commands? What do you think? How imperative is it to fix this now?
Validation ReportAll 20 validations passed. Show details
|
3 participants
What does this PR do?
Fixes ddev validate dep so unpinned dependencies are not flagged for integrations-extras and marketplace repos as errors. Unpinned dependencies should only be flagged for integrations-core.
An example of a dependency that got flagged before as unpinned for the marketplace repo:
[project.optional-dependencies]
deps = [
"azure-storage-blob>=12.14.1",
"google-cloud-storage>=2.7.0",
"boto3",
"botocore",
"paramiko",
"PySocks",
]
In integrations-extras and marketplace, integrations are distributed/managed differently, and many deps (especially optional ones) are not expected to be frozen into Agent requirements.
Motivation
AI-6341
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged