Merge lp:~hloeung/landscape-client-charm/show-charm-source-version into lp:landscape-client-charm

[Haw Loeung]

Add showing charm source version/commit in juju status output

[lp:~hloeung/landscape-client-charm/show-charm-source-version]

72. By Haw Loeung on 2021-02-04

Add showing charm source version/commit in juju status output

[Colin Misare (cmisare)]

LGTM, similar to https://code.launchpad.net/~hloeung/autocert-charm/+git/autocert-charm/+merge/397533

[Colin Misare (cmisare)]

review: Approve

[Simon Poirier (simpoir)]

Although I do like some useful debug info, I see a few small things:
1) Obvious but essential nitpick. please run `make lint`. Some lines are too long.
2) I see no reason to truncate revisions. This is not git and we're not saving saving bytes here.
3) The truncated version is useless as this is a bzr repo (e.g. <email address hidden> truncates to 'haw.loeu…')
4) This seems like it would be more useful in the debug logs than just in the ephemeral status line. What's the exact use case?

review: Needs Fixing

[lp:~hloeung/landscape-client-charm/show-charm-source-version]

73. By Haw Loeung on 2021-02-18

Fixed based on review

[Haw Loeung (hloeung)]

> Although I do like some useful debug info, I see a few small things:
> 1) Obvious but essential nitpick. please run `make lint`. Some lines are too
> long.

Fixed.

> 2) I see no reason to truncate revisions. This is not git and we're not saving
> saving bytes here.
> 3) The truncated version is useless as this is a bzr repo (e.g.
> <email address hidden> truncates to
> 'haw.loeu…')

It's shown in the juju status output, we usually use juju status --format=tabular so would be useful to try keep this short. I've fixed it now, it should show:

| 20200222-auxx19qq

Elsewhere, we're using this. e.g.:

| $ jsft | grep 'Ready.*source'
| apt-stresstest/0* active idle 3 52.224.81.63 Ready (source version/commit efcc988-…)
| ubuntu-repository-cache/0* active idle 0 52.146.31.10 80/tcp Ready (source version/commit 20210217-q3mg90b4)

> 4) This seems like it would be more useful in the debug logs than just in the
> ephemeral status line. What's the exact use case?

status_set() is also logged in the juju unit logs.

In our use case, we have periodic jobs to dump out juju status info. This allows us to use tools such as grep to check across multiple environments/models and find old and outdated charms to upgrade.

review: Needs Resubmitting

[Simon Poirier (simpoir)]

Thanks. LGTM.

Did you want to tell yourself to resubmit that MP or was that review status just by mistake?

review: Approve

[Simon Poirier (simpoir)]

FYI this is also published to the cs.

=== modified file 'hooks/common.py'
--- hooks/common.py	2019-02-03 14:50:36 +0000
+++ hooks/common.py	2021-02-18 02:38:19 +0000
@@ -85,6 +85,20 @@
     def local_unit(self):
         return local_unit()
 
+    def get_charm_version(self):
+        revision = ''
+        if os.path.exists('version'):
+            with open('version') as f:
+                line = f.readline().strip().split('-')
+            # We only want the first 8 characters, that's enough to tell which
+            # version of the charm we're using.
+            ver = line[-1][:8]
+            # We also want to include the date, again first 8 chars.
+            date = line[-2][:8]
+            revision = '{}-{})'.format(date, ver)
+
+        return revision
+
     def update_client_config(self, service_config, try_to_register=True):
         """Update the client config.
 
@@ -118,7 +132,12 @@
         if is_configured_enough:
             exit_code = self.try_to_register()
             if exit_code == 0:
-                self.status_set("active", "System successfully registered")
+                ver_str = ''
+                ver = self.get_charm_version()
+                if ver:
+                    ver_str = ' (source version/commit {})'.format(ver)
+                self.status_set("active", "System successfully registered{}"
+                                .format(ver_str))
         else:
             if not self.config.get("account_name"):
                 self.status_set(