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

pchila · 2023-08-30T16:17:38Z

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

Closes Error initializing version information: reading package version from file....package.version: no such file or directory #3285

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?
...

ycombinator · 2023-08-30T16:31:38Z

version/helper.go

+		}
+	}
+
+	return filepath.Join(dirPath, PackageVersionFileName), nil


Take a look at

elastic-agent/internal/pkg/agent/application/paths/common.go

Lines 211 to 221 in 454e625

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

elastic-agent/internal/pkg/agent/application/paths/common.go

Lines 229 to 241 in 454e625

// 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

elastic-agent/internal/pkg/agent/application/paths/common.go

Lines 206 to 208 in 454e625

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.

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)

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.

Created #3337

pchila · 2023-08-30T16:59:07Z

/cc @ruflin @pierrehilbert

elasticmachine · 2023-08-30T17:05:52Z

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

elasticmachine · 2023-08-30T17:14:08Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

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 · 2023-08-30T17:14:18Z

🌐 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`)	💚

ycombinator

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 · 2023-08-30T17:44:12Z

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

ycombinator · 2023-08-30T17:46:18Z

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

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

(cherry picked from commit 7dcc732)

…) (#3336) (cherry picked from commit 7dcc732) Co-authored-by: Paolo Chilà <[email protected]>

…stic#3335)

move the package version file outside of app directory for MacOS

73b4c7c

pchila added bug Something isn't working backport-v8.10.0 Automated backport with mergify labels Aug 30, 2023

pchila requested a review from a team as a code owner August 30, 2023 16:17

pchila self-assigned this Aug 30, 2023

pchila requested review from ycombinator and AndersonQ August 30, 2023 16:17

pchila added the skip-changelog label Aug 30, 2023

ycombinator reviewed Aug 30, 2023

View reviewed changes

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Aug 30, 2023

pchila requested a review from ycombinator August 30, 2023 17:14

ycombinator approved these changes Aug 30, 2023

View reviewed changes

pierrehilbert merged commit 7dcc732 into elastic:main Aug 30, 2023
13 of 16 checks passed

mergify bot pushed a commit that referenced this pull request Aug 30, 2023

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

e3e5a2e

(cherry picked from commit 7dcc732)

mergify bot mentioned this pull request Aug 30, 2023

[8.10](backport #3335) move the package version file outside of app directory for MacOS #3336

Merged

pchila mentioned this pull request Aug 30, 2023

Check if GetAgentPackageVersionFilePath should be merged with existing functions in paths package #3337

Open

cmacknz pushed a commit that referenced this pull request Aug 30, 2023

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

8a7242b

…) (#3336) (cherry picked from commit 7dcc732) Co-authored-by: Paolo Chilà <[email protected]>

cmacknz pushed a commit to cmacknz/elastic-agent that referenced this pull request Sep 25, 2023

move the package version file outside of app directory for MacOS (ela…

c84fa66

…stic#3335)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pchila commented Aug 30, 2023 •

edited

Loading

ycombinator Aug 30, 2023 •

edited

Loading

pchila Aug 30, 2023 •

edited

Loading

ycombinator Aug 30, 2023 •

edited

Loading

pchila Aug 30, 2023

pchila commented Aug 30, 2023

elasticmachine commented Aug 30, 2023

elasticmachine commented Aug 30, 2023

Build stats

Test stats 🧪

elasticmachine commented Aug 30, 2023

ycombinator left a comment

pierrehilbert commented Aug 30, 2023

ycombinator commented Aug 30, 2023

	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)
	}

	// 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
	}

	func initialTop() string {
	return ExecDir(retrieveExecutableDir())
	}

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

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

Conversation

pchila commented Aug 30, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

ycombinator Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

pchila Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

ycombinator Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

pchila Aug 30, 2023

Choose a reason for hiding this comment

pchila commented Aug 30, 2023

elasticmachine commented Aug 30, 2023

elasticmachine commented Aug 30, 2023

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Aug 30, 2023

🌐 Coverage report

ycombinator left a comment

Choose a reason for hiding this comment

Before this PR

After this PR

pierrehilbert commented Aug 30, 2023

ycombinator commented Aug 30, 2023

pchila commented Aug 30, 2023 •

edited

Loading

ycombinator Aug 30, 2023 •

edited

Loading

pchila Aug 30, 2023 •

edited

Loading

ycombinator Aug 30, 2023 •

edited

Loading