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

🚀 Restore :revision config #315

Merged
merged 7 commits into from
Mar 20, 2018
Merged

🚀 Restore :revision config #315

merged 7 commits into from
Mar 20, 2018

Conversation

jeffkreeftmeijer
Copy link
Member

Closes #215.

@jeffkreeftmeijer jeffkreeftmeijer self-assigned this Mar 2, 2018
@@ -214,6 +214,9 @@ defmodule Appsignal.Config do
unless empty?(config[:files_world_accessible]) do
System.put_env("_APPSIGNAL_FILES_WORLD_ACCESSIBLE", to_string(config[:files_world_accessible]))
end
unless empty?(config[:revision]) do
System.put_env("APP_REVISION", to_string(config[:revision]))
end
Copy link
Member

Choose a reason for hiding this comment

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

This env var should always be set, otherwise the value would persist across releases with the rolling restarts.

But the same problem probably exists for other config options as well.

@tombruijn
Copy link
Member

Moving back to WIP until the review comment is resolved.

@tombruijn tombruijn changed the title Restore :revision config 🚀 Restore :revision config Mar 8, 2018
@jeffkreeftmeijer
Copy link
Member Author

Fixed in f345629. Will fix the other environment variables in a separate PR.

Copy link
Member

@tombruijn tombruijn left a comment

Choose a reason for hiding this comment

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

Elixir code 👍
Requires a small change in the extension.

@@ -214,6 +214,7 @@ defmodule Appsignal.Config do
unless empty?(config[:files_world_accessible]) do
System.put_env("_APPSIGNAL_FILES_WORLD_ACCESSIBLE", to_string(config[:files_world_accessible]))
end
System.put_env("APP_REVISION", to_string(config[:revision]))
Copy link
Member

Choose a reason for hiding this comment

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

I think we may need to have this config option be accepted as "empty string == unset" as well in the extension (private link).

It now reports [2018-03-13T11:11:00 (extension) #92120][Debug] Initializing extension handler with app revision in appsignal.log when the value is empty, which is not correct.

@tombruijn
Copy link
Member

⚠️ Requires a new agent build before merging

tombruijn pushed a commit that referenced this pull request Mar 15, 2018
@tombruijn tombruijn force-pushed the revision-config branch 2 times, most recently from 4081cf1 to 4f039e0 Compare March 15, 2018 17:09
@tombruijn
Copy link
Member

@jeffkreeftmeijer rebased this PR on develop (because of the agent.exs file location change) and added the new agent build. Can you also verify this works?

@tombruijn
Copy link
Member

tombruijn pushed a commit that referenced this pull request Mar 19, 2018
@jeffkreeftmeijer
Copy link
Member Author

Found an issue where the :revision configuration overwrote the APP_REVISION environment variable (which should be leading). Fixed in 7dc25ad.

@tombruijn
Copy link
Member

Ah well.. That introduces the issue again of unsetting the config option during a hot reload, would not actually unset it. Only way to fix that is also have the agent listen to an underscored version.

@jeffkreeftmeijer
Copy link
Member Author

Great find! Fixed in https://github.com/appsignal/appsignal-agent/pull/328 and fa4c11d. @tombruijn could you build a new agent release and bump the version?

@tombruijn
Copy link
Member

@jeffkreeftmeijer new agent build added!

@tombruijn
Copy link
Member

Rebased to fix merge conflicts caused by merging #316

@jeffkreeftmeijer jeffkreeftmeijer merged commit d991f28 into develop Mar 20, 2018
@jeffkreeftmeijer jeffkreeftmeijer deleted the revision-config branch March 20, 2018 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants