-
-
Notifications
You must be signed in to change notification settings - Fork 9
Add building of RPMs and update default paths for DEB packages and command line parameters #110
Conversation
Added directory creation
My review applies not to the way the packages are built here. Instead, as a user of the built artifacts I can say that the CentOS packages that are built by the action in this branch work in my tests with cluster generation in T2. |
The paths described in the documentation do not match with the ones configured in the rpm. |
# Conflicts: # Cargo.toml
Also changed default paths for a lot of agent directories - this definitely needs calling out in the commit message! Config file is now empty, as all settings are used with their default values.
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.
I think I have found issues but I'm not 100% sure. Please double check.
.rpm/systemd/stackable-agent.service
Outdated
Restart=on-abort | ||
StandardOutput=journal | ||
StandardError=journal | ||
Environment="CONFIG_FILE=/etc/stackable-agent/agent.conf" |
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 correct?
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.
File is obsolete.
.rpm/stackable-agent.spec
Outdated
%define __os_install_post %{_dbpath}/brp-compress | ||
%define debug_package %{nil} | ||
%define _bindir /opt/stackable-agent | ||
%define _confdir /etc/stackable-agent |
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 both of these correct?
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 entire file is not needed anymore, I simply forgot to include deletion of the now unused .rpm folder in my commit.
@@ -0,0 +1,62 @@ | |||
%define __spec_install_post %{nil} |
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.
there are two stackable-agent.spec files. Which one is correct or are they both used?
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.
Removed the incorrect one.
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.
See previous review
I have addressed the comments from Lars.
I'm not entirely sure if this comment still needs addressing after all the path changes I included in this PR? |
This PR makes a number of changes to the agent for both RPM and DEB packages, as well as default values for command line options.
I would have liked to split this into separate PRs, but as all these things are tied together there was no clean way of separating this, so I decided to bunch everything together.
Agent Code:
In line with ADR011 default value for the following parameters was changed (old value in [] for reference).
RPM:
Converted build process from operators to build agent rpm.
DEB:
Changed paths to be in line with the new defaults specified above.
Both packages currently do not deliver a config file, as they run completely of the default values. We should look into packaging a template config perhaps.
I am not entirely happy with the current directory structure for packaging, as we have some duplication in here, but I think it will serve for now and is not worth the effort needed to make it nicer (which would also make it more complex).
Also, changes to paths which files are installed into currently require a bit of digging and changes in multiple files - again, this is not nice, but I'm not sure it is worth the effort of making it nicer.
I've created issue #108 with some open questions that do not necessarily need to be included in this PR but should be addressed at some point.