{"action":"created","issue":{"url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/183","repository_url":"https://api.github.com/repos/juju-solutions/charms.reactive","labels_url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/183/labels{/name}","comments_url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/183/comments","events_url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/183/events","html_url":"https://github.com/juju-solutions/charms.reactive/pull/183","id":335575091,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk3MjQxNDAy","number":183,"title":"Add upgrade.series.{pre,post} flags","user":{"login":"johnsca","id":406082,"node_id":"MDQ6VXNlcjQwNjA4Mg==","avatar_url":"https://avatars3.githubusercontent.com/u/406082?v=4","gravatar_id":"","url":"https://api.github.com/users/johnsca","html_url":"https://github.com/johnsca","followers_url":"https://api.github.com/users/johnsca/followers","following_url":"https://api.github.com/users/johnsca/following{/other_user}","gists_url":"https://api.github.com/users/johnsca/gists{/gist_id}","starred_url":"https://api.github.com/users/johnsca/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/johnsca/subscriptions","organizations_url":"https://api.github.com/users/johnsca/orgs","repos_url":"https://api.github.com/users/johnsca/repos","events_url":"https://api.github.com/users/johnsca/events{/privacy}","received_events_url":"https://api.github.com/users/johnsca/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-06-25T21:30:03Z","updated_at":"2018-06-26T09:52:22Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/juju-solutions/charms.reactive/pulls/183","html_url":"https://github.com/juju-solutions/charms.reactive/pull/183","diff_url":"https://github.com/juju-solutions/charms.reactive/pull/183.diff","patch_url":"https://github.com/juju-solutions/charms.reactive/pull/183.patch"},"body":"This also adds documentation for all existing automatically managed flags."},"comment":{"url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/comments/400250081","html_url":"https://github.com/juju-solutions/charms.reactive/pull/183#issuecomment-400250081","issue_url":"https://api.github.com/repos/juju-solutions/charms.reactive/issues/183","id":400250081,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMDI1MDA4MQ==","user":{"login":"stub42","id":2928805,"node_id":"MDQ6VXNlcjI5Mjg4MDU=","avatar_url":"https://avatars0.githubusercontent.com/u/2928805?v=4","gravatar_id":"","url":"https://api.github.com/users/stub42","html_url":"https://github.com/stub42","followers_url":"https://api.github.com/users/stub42/followers","following_url":"https://api.github.com/users/stub42/following{/other_user}","gists_url":"https://api.github.com/users/stub42/gists{/gist_id}","starred_url":"https://api.github.com/users/stub42/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stub42/subscriptions","organizations_url":"https://api.github.com/users/stub42/orgs","repos_url":"https://api.github.com/users/stub42/repos","events_url":"https://api.github.com/users/stub42/events{/privacy}","received_events_url":"https://api.github.com/users/stub42/received_events","type":"User","site_admin":false},"created_at":"2018-06-26T09:52:22Z","updated_at":"2018-06-26T09:52:22Z","author_association":"COLLABORATOR","body":"Another issue I just realized with requiring the charm to clear the flags is how does a charm start using them safely?\r\n\r\nI imagine this is how you hope to see them used:\r\n```python\r\n@when('upgrade.series.pre')\r\ndef prepare_dist_upgrade():\r\n    host.service_stop('foo')\r\n    apt.purge('foo')\r\n    reactive.clear_flag('upgrade.series.pre')\r\n```\r\n\r\nThe problem is, what if there had previously been a distro upgrade on this unit? The flag will have already been set and not cleared, because the charm didn't care before. When the new code lands to support upgrades better, the handler will run and maybe harmful things happen because a distro upgrade isn't actually happening; the flags were set by charms.reactive, and clearing them deferred to a charm that didn't care. And when the charm starts caring, it lacks information to handle the flag lifecycle correctly.\r\n\r\nYou can't resolve the situation n a `@hook('upgrade-charm')` or `@when('hook.upgrade-charm')` handler, as handler does not know what state the system was in when the flag got set, so does not know if it should be cleared. It can either leave the flag set and hope the handlers are idempotent, or clear the flags and risk skipping handlers that should be run. Triggers, however, do provide an option, provided charms.reactive always clears the flag and resets it, causing triggers to fire even if the flag was already set. Which is adding more confusion, because you now have some flags that fire their triggers even when the flag does not change state, which is not the default behavior.\r\n\r\nI think this applies to all reactive managed flags to some extent, including the most common like config.changed. It pretty much mandates that handlers using them for guards must be idempotent, or requires using triggers in all cases rather than just in base layers.\r\n\r\n```python\r\nregister_trigger(when='upgrade.series.pre', set_flag='my.upgrade.series.pre')\r\n\r\n@when('my.upgrade.series.pre')\r\ndef prepare_dist_upgrade():\r\n    host.service_stop('foo')\r\n    apt.purge('foo')\r\n    reactive.clear_flag('my.upgrade.series.pre')\r\n```\r\n\r\nI'm not feeling that this direction change is for the better. I think things are getting worse for charm authors. and it will be harder to write correct charms than it is now. There is more to understand, not less. If we must avoid 'set for the duration of the hook', then I think the problems can be avoided by setting and immediately clearing the flag. Charm authors would be required to use register_trigger, which would do the right thing. I think this is preferable to allowing charm authors to use alternative mechanisms, which are potentially foot guns. Best practice would become 'always use register_trigger', and we end up in the same situation but with more confusing documentation and needing to maintain the behavior for backwards compatibility.\r\n\r\nI also personally don't believe that 'set for the duration of the hook' is actually a problem, and that charm authors are quite able to understand and reason about what this means. I personally think the best way forward is to keep the flag lifecycles as they are, where they are cleared automatically at the end of the hook. Documented best practice would be to use register_trigger, because the pattern avoids the various race conditions entirely. We end up with simpler and more correct charms using a single, no matter if reading code in a charm layer or a base layer. And we never have to worry about changing how the charms.reactive flag lifecycles are maintained and backwards compatibility, because it doesn't matter when following best practice.\r\n\r\nI think we would be better off just documenting how to use triggers, and maybe coming up with easier ways to use them, making the flag lifecycle changes moot. \r\n\r\n\r\n"}}
{"actor":{"display_login":"stub42"}}