charm-sysconfig

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

  1. Git
  2. lp:~dashmage/charm-sysconfig
  3. bug-2012581/clear-notification-fix
  4. Merge into master
Proposed by Ashley James
Status: Merged
Approved by: Eric Chen
Approved revision: 9dad38769b4bcc7b9072e6ae7c77a38d34fa59d9
Merged at revision: 6f88d54f4e62c85ea9fd2595ab45f3f6d3d64db0
Proposed branch: ~dashmage/charm-sysconfig:bug-2012581/clear-notification-fix
Merge into: charm-sysconfig:master
Diff against target: 32 lines (+3/-2)
2 files modified
src/lib/lib_sysconfig.py (+2/-1)
src/tests/functional/test_deploy.py (+1/-1)
Reviewer Review Type Date Requested Status
JamesLin Approve
🤖 prod-jenkaas-bootstack continuous-integration Approve
Mert Kirpici (community) Approve
BootStack Reviewers Pending
BootStack Reviewers Pending
Review via email: mp+440819@code.staging.launchpad.net

Commit message

Fix clear-notification action

- Also fixes broken functional tests

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
Robert Gildein (rgildein) wrote :

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

Revision history for this message
JamesLin (jneo8) wrote :

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

review: Needs Information
Revision history for this message
Ashley James (dashmage) wrote :

> 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.

Revision history for this message
Mert Kirpici (mertkirpici) wrote :

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

review: Approve
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

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)
Revision history for this message
Eric Chen (eric-chen) wrote :

> > 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.

Revision history for this message
JamesLin (jneo8) wrote :

LGTM

review: Approve
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision 6f88d54f4e62c85ea9fd2595ab45f3f6d3d64db0

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: