-
Notifications
You must be signed in to change notification settings - Fork 158
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
Set env vars for service instead of machine #3930
Set env vars for service instead of machine #3930
Conversation
9647687
to
e091ad7
Compare
Returning to draft until I can look at the test failure. |
The msi itself also needs to be updated: https://github.com/signalfx/splunk-otel-collector/blob/main/internal/buildscripts/packaging/msi/splunk-otel-collector.wxs#L42 |
And we'll have to update our ansible, chef, and puppet modules since they currently only configure |
- name: Assert splunk-otel-collector service status | ||
assert: | ||
that: not service_status.changed | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removal related to the env var changes? Wonder if there should be a new var to dictate if expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was failing consistently for me because the status had changed to "started" - I confess that I don't understand how it was working before.
@@ -37,7 +37,6 @@ | |||
ansible.windows.win_package: | |||
path: "{{otel_msi_package.dest}}" | |||
state: present | |||
notify: "restart windows splunk-otel-collector" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these removals related and necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the restart should only happen after the environment variables are set for the service.
Co-authored-by: Ryan Fitzpatrick <10867373+rmfitzpatrick@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, feel free to merge if enough CI coverage.
Description:
On Windows move the environment variables to the service scope. This servers two purposes:
Thanks @pellared for noticing this issue.
Review notes
"Monkey see, monkey does": in retrospect, after learning a bit about some of the tech used here, I wanted to change some other things, e.g.: use an array instead of maps that a later converted to arrays in Chef, but, this is already taking too long and such improvements can be addressed separately.
Sorting variables when writing to the registry: this facilitates human inspection and tests.
Choco: if there was a previous version with environment variables on the machine scope, the environment variables are removed from the machine level. Not doing the same for other deployments because they don't seem to support upgrading deployments, even the install script requires removing the previously installed collector.
Ansible:
deployments/ansible/roles/collector/tasks/otel_win_install.yml
, the restart is done only after the environment variables for the service are updated.Puppet:
deployments/puppet/manifests/collector_win_registry.pp
: respective tests are atinternal\buildscripts\packaging\tests\deployments\puppet\puppet_test.py
(internal/buildscripts/packaging/tests/helpers/win_utils.py
was modified to support new environment variables location).Link to Splunk idea:
N/A
Testing:
Local tests and CI.
Documentation:
Updating CHANGELOG.md
cc @RassK @theletterf