(SDI-2199) Fix #1344 binary change to snaptel/snapteld #1357

nanliu · 2016-11-17T01:00:57Z

No description provided.

nanliu · 2016-11-17T01:34:10Z

snapteld.go

@@ -109,7 +109,7 @@ const (
 	defaultLogPath     string = ""
 	defaultLogTruncate bool   = false
 	defaultLogColors   bool   = true
-	defaultConfigPath  string = "/etc/snap/snapd.conf"
+	defaultConfigPath  string = "/etc/snap/snapteld.conf"


I've made a note to symlink this as well in the packaging since this will be a breaking upgrade.

IRCody · 2016-11-17T23:28:14Z

README.md

@@ -121,24 +121,22 @@ Tarball (choose the appropriate version and platform):
 ```
 $ curl -sfLO https://github.com/intelsdi-x/snap/releases/download/0.18.0/snap-0.18.0-linux-amd64.tar.gz
 $ tar xf snap-0.18.0-linux-amd64.tar.gz
-$ cp snapd /usr/local/bin
-$ cp snapctl /usr/local/bin
+$ cp snapteld /usr/local/sbin


Does it make sense to put snapteld and snaptel in separate locations?

Yes, same as /usr/sbin/sshd and /usr/bin/ssh.

IRCody · 2016-11-17T23:30:45Z

README.md

 ```

-Ubuntu 16.04.1 [snapd package version 2.13+](https://launchpad.net/ubuntu/+source/snapd) installs snapd/snapctl binary in /usr/bin. These executables are not related to snap-telemetry. Running `snapctl` from snapd package will result in the following error message:
+If you installed snap-telemetry package, please make sure `/usr/local/bin:/usr/local/sbin` is part of your $PATH and has higher priority than `/usr/bin:/usr/sbin`, or invoke the snap-telemetry snapteld/snaptel binary using fully qualified path.


Is this referring to older versions only? Versions post-name change should be fine regardless of where /usr/local/(s)bin is in the path.

This can be dropped with the new binary name so I'll remove it.

IRCody · 2016-11-17T23:32:35Z

README.md

@@ -157,15 +155,15 @@ $ systemctl start snap-telemetry
 If you installed Snap from binary, you can start Snap daemon via the command:
 ```
 $ sudo mkdir -p /var/log/snap
-$ sudo snapd --plugin-trust 0 --log-level 1 -o /var/log/snap &
+$ sudo snapteld --plugin-trust 0 --log-level 1 -o /var/log/snap &


Should we change the -o to --log-path to match the other self-explanatory options?

IRCody · 2016-11-17T23:40:36Z

docs/SNAPTELD_CONFIGURATION.md


-snapd supports being configured through a configuration file located at a default location of `/etc/snap/snapd.conf` on Linux systems or by passing a configuration file in through the `--config` command line flag when starting snapd. YAML and JSON are currently supported for configuration file types.
+snapteld supports being configured through a configuration file located at a default location of `/etc/snap/snapteld.conf` on Linux systems or by passing a configuration file in through the `--config` command line flag when starting snapteld. YAML and JSON are currently supported for configuration file types.


Is the config file location going to stay as /etc/snap/ vs /etc/snaptel?

I think we can keep it /etc/snap since there's no conflicts. This will allow keyrings file to be kept in same location as well.

IRCody · 2016-11-17T23:42:44Z

snapteld.go

@@ -438,7 +438,7 @@ func action(ctx *cli.Context) error {
 							log.WithFields(
 								log.Fields{
 									"block":       "main",
-									"_module":     "snapd",
+									"_module":     "snapteld",


Instead of changing these all from snapd->snapteld, would it make sense to just instantiate a logger with the module set in one place? Or at least use a moduleName var?

kindermoumoute · 2016-11-18T00:09:37Z

docs/SNAPTELD_CONFIGURATION.md

@@ -17,11 +17,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->



Do we want to keep this name for this file? What about docs/DAEMON_CONFIGURATION.md?

I'm going to defer any documentation link changes to SDI-2203. The goal for this PR is to fix the snaptel/snapteld binary, and have the installation steps match the new package location (sbin/bin).

If we have any documentation touch ups, they can be done post 0.19.0 release.

kindermoumoute · 2016-11-18T22:44:21Z

I guess the GLOSSARY.md has to be updated too now it's merged.

mbbroberg · 2016-11-18T23:30:44Z

Correct @kindermoumoute - whatever naming we choose for the file will need replacement in the glossary as well.

nanliu · 2016-11-19T00:57:49Z

Once this is finalized, I'll squash it into one commit before merging. We plan to merge this on Monday before the 0.19.0 release.

lmroz · 2016-11-21T12:07:00Z

README.md

 ```

-Ubuntu 16.04.1 [snapd package version 2.13+](https://launchpad.net/ubuntu/+source/snapd) installs snapd/snapctl binary in /usr/bin. These executables are not related to snap-telemetry. Running `snapctl` from snapd package will result in the following error message:
+NOTE: snap-telemetry packages prior to 0.19.0 installed `/usr/local/bin/{snapctl|snapd}` and these binaries have been renamed to `snaptel` and `snapteld`. snap-telemetry packages prior to 0.18.0 symlinked `/usr/bin/{snapctl|snapd}` to `/opt/snap/bin/{snapctl|snapd}` and may cause conflicts with [Ubuntu's `snapteld` package](http://packages.ubuntu.com/xenial-updates/snapd). Ubuntu 16.04.1 [snapteld package version 2.13+](https://launchpad.net/ubuntu/+source/snapd) installs snapd/snapctl binary in /usr/bin. These executables are not related to snap-telemetry. Running `snapctl` from snapd package will result in the following error message:


and may cause conflicts with [Ubuntu's >>>>>snapteld<<< package]

>>>snapteld<<< package version 2.13+

lmroz · 2016-11-21T12:07:26Z

README.md


 ```
 $ snapctl
-error: snapctl requires SNAP_CONTEXT environment variable
+error: snaptel requires SNAP_CONTEXT environment variable


Revert this.

lmroz · 2016-11-21T12:16:27Z

There is /etc/snap/snapteld.conf but /etc/snapteld/plugins/ (CTRL+F for /etc/, I don't remember exact location).

nanliu · 2016-11-21T17:29:36Z

@lmroz found /etc/snapteld/plugins and changed it to /opt/snap/plugins since that's our package default. Thanks for tracking down all the inconsistencies.

kindermoumoute

LGTM

nanliu commented Nov 17, 2016

View reviewed changes

nanliu force-pushed the SDI-2199 branch from 35e061a to a53de3c Compare November 17, 2016 01:35

nanliu added breaking-change/cli wip-do-not-merge labels Nov 17, 2016

nanliu force-pushed the SDI-2199 branch 3 times, most recently from ea33005 to e07a429 Compare November 17, 2016 22:34

IRCody reviewed Nov 17, 2016

View reviewed changes

kindermoumoute reviewed Nov 18, 2016

View reviewed changes

nanliu force-pushed the SDI-2199 branch from 088b6e9 to 23841ed Compare November 19, 2016 00:42

lmroz reviewed Nov 21, 2016

View reviewed changes

(SDI-2199) Fix intelsdi-x#1344 binary change to snaptel/snapteld

d1c48a8

nanliu force-pushed the SDI-2199 branch from 23841ed to d1f9790 Compare November 21, 2016 17:21

(SDI-2199) snaptel/snapteld documentation changes

fc2219a

nanliu force-pushed the SDI-2199 branch from d1f9790 to fc2219a Compare November 21, 2016 17:26

nanliu removed the wip-do-not-merge label Nov 21, 2016

kindermoumoute approved these changes Nov 21, 2016

View reviewed changes

nanliu merged commit cd9c1a8 into intelsdi-x:master Nov 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SDI-2199) Fix #1344 binary change to snaptel/snapteld #1357

(SDI-2199) Fix #1344 binary change to snaptel/snapteld #1357

nanliu commented Nov 17, 2016

nanliu Nov 17, 2016

IRCody Nov 17, 2016

nanliu Nov 18, 2016

IRCody Nov 17, 2016

nanliu Nov 18, 2016

IRCody Nov 17, 2016

IRCody Nov 17, 2016

nanliu Nov 18, 2016

IRCody Nov 17, 2016

kindermoumoute Nov 18, 2016

nanliu Nov 19, 2016

kindermoumoute commented Nov 18, 2016

mbbroberg commented Nov 18, 2016

nanliu commented Nov 19, 2016

lmroz Nov 21, 2016

lmroz Nov 21, 2016

lmroz commented Nov 21, 2016 •

edited

Loading

nanliu commented Nov 21, 2016

kindermoumoute left a comment


		snapd supports being configured through a configuration file located at a default location of `/etc/snap/snapd.conf` on Linux systems or by passing a configuration file in through the `--config` command line flag when starting snapd. YAML and JSON are currently supported for configuration file types.
		snapteld supports being configured through a configuration file located at a default location of `/etc/snap/snapteld.conf` on Linux systems or by passing a configuration file in through the `--config` command line flag when starting snapteld. YAML and JSON are currently supported for configuration file types.

		@@ -17,11 +17,11 @@ See the License for the specific language governing permissions and
		limitations under the License.
		-->

(SDI-2199) Fix #1344 binary change to snaptel/snapteld #1357

(SDI-2199) Fix #1344 binary change to snaptel/snapteld #1357

Conversation

nanliu commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindermoumoute commented Nov 18, 2016

mbbroberg commented Nov 18, 2016

nanliu commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmroz commented Nov 21, 2016 • edited Loading

nanliu commented Nov 21, 2016

kindermoumoute left a comment

Choose a reason for hiding this comment

lmroz commented Nov 21, 2016 •

edited

Loading