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

Best effort copying of run directory #2448

Merged
merged 10 commits into from Apr 6, 2023

Conversation

michalpristas
Copy link
Contributor

What does this PR do?

This PR is using onErr hook provided by copy utility.
This onErr is raised whenever tool encounters an error, it's up to a caller to decide what to do with the error. In this PR we log error and return nil (swallowing the error) resulting in a copy process not being terminated.

onErr is a custom hook introuced in my own fork of the tool (hence replace in go.mod)
I also opened a PR with this hook (fully tested) in copy repo: otiai10/copy#101
Once they decide to merge this we can remove our replace.

The other option was to perform copying by ourselves but the tool also handles symlinks and other edge cases already.

Unit tests for open handles also present.

Why is it important?

Prevents upgrade to fail on copy.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

Fixes: #2433

@michalpristas michalpristas added bug Something isn't working Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team backport-v8.7.0 Automated backport with mergify labels Apr 4, 2023
@michalpristas michalpristas self-assigned this Apr 4, 2023
@michalpristas michalpristas requested a review from a team as a code owner April 4, 2023 13:46
@michalpristas michalpristas requested review from michel-laterman and removed request for a team April 4, 2023 13:46
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 4, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-04-05T13:53:55.630+0000

  • Duration: 19 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 5411
Skipped 23
Total 5434

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 4, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.485% (65/66) 👍
Files 69.604% (158/227) 👍
Classes 68.736% (299/435) 👍
Methods 53.915% (909/1686) 👍
Lines 39.052% (10179/26065) 👍 0.023
Conditionals 100.0% (0/0) 💚

Comment on lines 92 to 94
if fl != nil {
require.NoError(t, fl.Unlock())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe invoke this via a defer so it will get executed even if the require.Equal on the previous line fails?

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM (pending @cmacknz's review feedback).

@mergify
Copy link
Contributor

mergify bot commented Apr 4, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix/upgrade-run-err upstream/fix/upgrade-run-err
git merge upstream/main
git push upstream fix/upgrade-run-err

@michalpristas michalpristas merged commit df3d52d into elastic:main Apr 6, 2023
17 checks passed
mergify bot pushed a commit that referenced this pull request Apr 6, 2023
Best effort copying of `run` directory (#2448)

(cherry picked from commit df3d52d)
michalpristas added a commit that referenced this pull request Apr 6, 2023
Best effort copying of `run` directory (#2448)

(cherry picked from commit df3d52d)

Co-authored-by: Michal Pristas <michal.pristas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.7.0 Automated backport with mergify bug Something isn't working Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
5 participants