Merge ~dashmage/charm-sysconfig:bug-2012581/clear-notification-fix into charm-sysconfig:master

[Ashley James]

Fix clear-notification action

- Also fixes broken functional tests

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Robert Gildein (rgildein)]

LGTM, but can you add simple unit tests for those two functions. Thanks

[JamesLin (jneo8)]

One inline comment, could you also provide more information why this fix work?

review: Needs Information

[Ashley James (dashmage)]

> Why we need to flush local's unit data here? Also the previous line is `set`, flush after set look make not sense to me.
> Could you explain why we need to do this?

So without explicitly adding `unitdata.kv().flush()` it seems that the local unit's data is not being written to the DB. Without this line, when we try to get the value later, it's only returning `None`.

For the other line, `datetime.fromtimestamp` is the correct function to convert the POSIX timestamp (in float) to the local date.

[Mert Kirpici (mertkirpici)]

I verified that the action AND the functest was broken and now they both work properly with this change. Nice catch, thanks!

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:9dad38769b4bcc7b9072e6ae7c77a38d34fa59d9
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/10/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/609/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/215/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-sysconfig-ci/10//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

> > Why we need to flush local's unit data here? Also the previous line is
> `set`, flush after set look make not sense to me.
> > Could you explain why we need to do this?
>
> So without explicitly adding `unitdata.kv().flush()` it seems that the local
> unit's data is not being written to the DB. Without this line, when we try to
> get the value later, it's only returning `None`.
>
> For the other line, `datetime.fromtimestamp` is the correct function to
> convert the POSIX timestamp (in float) to the local date.

https://github.com/juju/charm-helpers/blob/master/charmhelpers/core/unitdata.py#L165

Modifications are not persisted unless :meth:`flush` is called.

https://github.com/juju/charm-helpers/blob/master/charmhelpers/contrib/hardening/audits/file.py#L423
I can find similar usage here.

[JamesLin (jneo8)]

LGTM

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 6f88d54f4e62c85ea9fd2595ab45f3f6d3d64db0

diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py
index facdd53..1a9875a 100644
--- a/src/lib/lib_sysconfig.py
+++ b/src/lib/lib_sysconfig.py
@@ -73,7 +73,7 @@ def clear_notification_time():
     clear_notification_timestamp = unitdata.kv().get("clear-notification-timestamp")
     if not clear_notification_timestamp:
         return None
-    return datetime.fromordinal(clear_notification_timestamp)
+    return datetime.fromtimestamp(clear_notification_timestamp, tz=timezone.utc)
 
 
 def check_update_grub(tmp_output="/tmp/tmp_grub.cfg"):
@@ -117,6 +117,7 @@ def clear_notification():
     """
     timestamp = datetime.now(timezone.utc)
     unitdata.kv().set("clear-notification-timestamp", timestamp.timestamp())
+    unitdata.kv().flush()
     message = "Notifications cleared at {}".format(timestamp.isoformat())
     hookenv.log(message, hookenv.DEBUG)
 
diff --git a/src/tests/functional/test_deploy.py b/src/tests/functional/test_deploy.py
index f1cd22b..c97845e 100644
--- a/src/tests/functional/test_deploy.py
+++ b/src/tests/functional/test_deploy.py
@@ -307,7 +307,7 @@ async def test_clear_notification_persist_after_update_status(app, model):
     unit = app.units[0]
     action = await unit.run_action("clear-notification")
     action = await action.wait()
-    action = await unit.run("./hooks/unpdate-status")
+    action = await unit.run("JUJU_HOOK_NAME=update-status ./hooks/update-status")
     await model.block_until(lambda: app.status == "active", timeout=TIMEOUT)