Improve DRA packages testing in PRs #11953
Conversation
blakerouse
added
Team:Elastic-Agent-Control-Plane
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
.buildkite/scripts/steps/package.sh
Outdated
| # The DRA process only pulls already built elastic-agent-core packages any binaries | ||
| # or files that have been placed in build/ during the running of packageAgentCore | ||
| # will not be present. This ensures the same environment for PR's as the DRA path. | ||
| find build/ -mindepth 1 -maxdepth 1 ! -name 'distributions' -exec rm -rf {} + |
There was a problem hiding this comment.
That this is necessary feels like there is a bug in how we pick up files, if we'll just accept whatever is in the build directory.
Not opposed to this to unstick things, but this feels like it is more of a work around.
There was a problem hiding this comment.
This isn't to unstick anything. The job passes on PR's already. The issue is that the DRA process is not the same as a PR process. A PR process has to build the core binaries, the DRA process pulls the core binaries from another pipeline. This just ensures the filesystem contents are the same.
The DRA process was also successful so there are no packaging issues at the moment left, the build is working - https://buildkite.com/elastic/elastic-agent-package/builds/8918
The goal of this change is to ensure that we catch issues like @swiatekm fixed here - #11940
If this change was present the PR I was working on to fix the DRA process would have failed and it would have been caught before the DRA process occurred. The goal of this change is to ensure that in the future changes to files or package building is caught in PR's before they are merged and the DRA process fails.
There was a problem hiding this comment.
Thanks, thinking more, just forcing a clean slate makes sense, because if you think you've setup the paths correctly but haven't (or are doing a clean build but aren't) there's no other way to catch this.
This still smells like something is fishy with where our build output goes, e.g. I would have trouble seeing a Makefile target that has a clean as an intermediate step and thinking it was doing things correctly.
There was a problem hiding this comment.
Why don't we also need to do this locally? If I package agent core first by itself I end up with:
❯ ls build
data golang-crossbuild package
distributions mage-linux-arm64 windows-amd64-archive-root-binary
❯ ls build/distributions
elastic-agent-core-9.4.0-SNAPSHOT-windows-x86_64.zip
elastic-agent-core-9.4.0-SNAPSHOT-windows-x86_64.zip.sha512
The problem was that we were able to pick up windows-amd64-archive-root-binary instead of picking it out of elastic-agent-core-9.4.0-SNAPSHOT-windows-x86_64.zip?
There was a problem hiding this comment.
OK so looking again at https://github.com/elastic/elastic-agent/pull/11940/changes, we were able to have the build pass in CI by using the wrong path which doesn't exist if you just download the core artifact.
So the only way to not have that mistake is to delete that file so we can make it similar to if we never built it.
There was a problem hiding this comment.
Could this instead be part of the packageAgentCore target? Where it deletes everything except the build/distributions directory like we do here?
That would make local behave the same as CI.
There was a problem hiding this comment.
I don't think we should make it part of packageAgentCore, because after running that it helps to inspect the files and there location. I could see making a dedicated target to remove the files, and adding that target after packageAgentCore and then before the next step in the package target for developers.
I will update this PR to do that. I am also looking at the version bump failure that is now happening.
| if mg.Verbose() { | ||
| fmt.Println("removed", absBuildDir) | ||
| for _, b := range testBinariesPath { | ||
| fmt.Println("removed", b) |
There was a problem hiding this comment.
This was removed because it wasn't actually removing anything, it was just printing that it was...
4 participants
What does this PR do?
Updates the package.sh script that is used for the DRA process and triggered for PR's. This update ensures that when triggered from a PR, after
packageAgentCoreis ran the entirebuilddirectory is removed except forbuild/distributions. This provides a consistent directory structure for a PR and the DRA process.The DRA process will only have the elastic-agent core packages and no other binaries in place.
Why is it important?
To provide consistency between PR's and the DRA. We want to ensure that by the time the DRA job runs it passes, for all changes introduced.
Checklist