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

Run full dependency validation on PR as well as master #15505

Closed
wants to merge 1 commit into from

Conversation

iliakur
Copy link
Contributor

@iliakur iliakur commented Aug 8, 2023

What does this PR do?

Before

We would pass the changed integration eg ddev validate dep rabbitmq when running on PR.

After

Both for PRs and for master we run ddev validate dep.

Motivation

  • this PR disables validating changes in deps for an individual check against agent_requirements.in. The logic is that these errors will be caught soon enough by the master CI. Unfortunately this can result in a problem: we break the agent build and there's a lot of pressure to fix our master CI.
  • Case: this PR changed an important dependency. The CI on the PR passed, then failed on master and as soon as we updated agent_requirements.in we discovered that we broke the agent build. It would have been useful to check agent_requirements.in in the first PR and deal with agent build problems earlier.
  • When running locally I didn't observe a noticeable slowdown for running the validation on all integrations vs only a single one. I thought the tradeoff is good between simplicity and performance if I just switch the CI to check agent requirements on every PR.

Alternative

We could try to address the original problem, by considering only a subset of agent_requirements.in deps that's relevant to a given integration. This is the proper solution, but it looks like more work. In my opinion for insufficient performance gain. We could potentially keep the current fix while creating a card for the tools squad to implement the proper fix.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@iliakur
Copy link
Contributor Author

iliakur commented Aug 8, 2023

Haha, the "proper fix" has actually already been implemented here and supplemented here

@iliakur iliakur closed this Aug 8, 2023
@dd-devflow dd-devflow bot deleted the ik/validate-agent-deps-on-pr branch February 8, 2024 00:00
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.

1 participant