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

move the package version file outside of app directory for MacOS #3335

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

pchila
Copy link
Member

@pchila pchila commented Aug 30, 2023

What does this PR do?

Move elastic package version file outside of elastic-agent.app directory to fix the package signing for Mac

Why is it important?

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

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@pchila pchila added bug Something isn't working backport-v8.10.0 Automated backport with mergify labels Aug 30, 2023
@pchila pchila requested a review from a team as a code owner August 30, 2023 16:17
@pchila pchila self-assigned this Aug 30, 2023
}
}

return filepath.Join(dirPath, PackageVersionFileName), nil
Copy link
Contributor

@ycombinator ycombinator Aug 30, 2023

Choose a reason for hiding this comment

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

Take a look at

func retrieveExecutableDir() string {
execPath, err := os.Executable()
if err != nil {
panic(err)
}
evalPath, err := filepath.EvalSymlinks(execPath)
if err != nil {
panic(err)
}
return filepath.Dir(evalPath)
}
and also
// ExecDir returns the "executable" directory which is:
// 1. The same if the execDir is not inside of the data path
// 2. Two levels up if the execDir inside of the data path on non-macOS platforms
// 3. Five levels up if the execDir inside of the dataPath on macOS platform
func ExecDir(execDir string) string {
if isInsideData(execDir) {
execDir = filepath.Dir(filepath.Dir(execDir))
if runtime.GOOS == darwin {
execDir = filepath.Dir(filepath.Dir(filepath.Dir(execDir)))
}
}
return execDir
}
and how they two are used together in
func initialTop() string {
return ExecDir(retrieveExecutableDir())
}

If I'm reading the logic of the GetAgentPackageVersionFilePath function, it looks pretty much the same as the logic in the functions mentioned above. So maybe you could reuse them somehow, either by exporting retrieveExecutableDir() from the paths package or by moving GetAgentPackageVersionFilePath() into the paths package? This would also let you get rid of the getCurrentExecutablePath function as I believe it's only being called from the GetAgentPackageVersionFilePath function.

Copy link
Member Author

@pchila pchila Aug 30, 2023

Choose a reason for hiding this comment

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

In this patch I am not making any assumption as to what the directory structure is... I just cut off the part of the path that goes in elastic-agent.app
the code we have in paths requires that there are exactly 0, 2 or 5 levels (for Mac) between the executable and the execDir.

The initial idea of this was to have the package version file side by side with the executable wherever it is... we are just moving it for mac because of package signing issues and I would prefer to keep changes to a minimum for this hotfix. We can open a follow-up issue if you feel that we should remove the duplication (we still have to verify that everything still works also for packages that we just extract somewhere on disk as it must not depend on installing agent)

Copy link
Contributor

@ycombinator ycombinator Aug 30, 2023

Choose a reason for hiding this comment

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

Sounds fair. Would you mind opening that follow up issue and linking it here so we don't lose track of this work? We really should ideally have only one code path through which paths are determined. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #3337

@pchila
Copy link
Member Author

pchila commented Aug 30, 2023

/cc @ruflin @pierrehilbert

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Aug 30, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@elasticmachine
Copy link
Contributor

💚 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-08-30T16:21:05.849+0000

  • Duration: 53 min 56 sec

Test stats 🧪

Test Results
Failed 0
Passed 6233
Skipped 55
Total 6288

💚 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
Contributor

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.765% (80/81) 👍
Files 67.368% (192/285) 👍
Classes 66.105% (353/534) 👍
Methods 53.017% (1107/2088) 👍
Lines 38.598% (12632/32727) 👎 -0.015
Conditionals 100.0% (0/0) 💚

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.

Tested this PR locally on my Mac.

Before this PR

After installing Agent:

$ sudo find /Library/Elastic/Agent -iname package.version
/Library/Elastic/Agent/data/elastic-agent-454e62/elastic-agent.app/Contents/MacOS/package.version
$ sudo cat /Library/Elastic/Agent/data/elastic-agent-454e62/elastic-agent.app/Contents/MacOS/package.version
8.11.0
$ sudo elastic-agent version
Binary: 8.11.0-SNAPSHOT (build: 454e6253a0e3457828b769e7c2c7c0e9365d870b at 2023-08-30 17:38:19 +0000 UTC)
Daemon: 8.11.0-SNAPSHOT (build: 454e6253a0e3457828b769e7c2c7c0e9365d870b at 2023-08-30 17:38:19 +0000 UTC)

After this PR

After installing Agent:

$ sudo find /Library/Elastic/Agent -iname package.version
/Library/Elastic/Agent/data/elastic-agent-73b4c7/package.version
$ sudo cat /Library/Elastic/Agent/data/elastic-agent-73b4c7/package.version
8.11.0
$ sudo elastic-agent version
Binary: 8.11.0-SNAPSHOT (build: 73b4c7cfaef362452cac4bd03e19518ed61212d5 at 2023-08-30 17:35:04 +0000 UTC)
Daemon: 8.11.0-SNAPSHOT (build: 73b4c7cfaef362452cac4bd03e19518ed61212d5 at 2023-08-30 17:35:04 +0000 UTC)

@pierrehilbert
Copy link
Contributor

Thx @ycombinator, I'm merging this to unlock the next BC.
@pchila don't forget to babysit the backport to 8.10 branch

@pierrehilbert pierrehilbert merged commit 7dcc732 into elastic:main Aug 30, 2023
13 of 16 checks passed
@ycombinator
Copy link
Contributor

@pchila don't forget to babysit the backport to 8.10 branch

@pchila I can take care of this if your day is ending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.10.0 Automated backport with mergify bug Something isn't working skip-changelog Team:Elastic-Agent Label for the Agent team
Projects
None yet
4 participants