{"action":"created","comment":{"url":"https://api.github.com/repos/dcos/dcos/pulls/comments/182732450","pull_request_review_id":113587904,"id":182732450,"diff_hunk":"@@ -264,13 +259,45 @@ def make_file(name):\n \n \n def write_json(filename, data):\n-    with open(filename, \"w+\") as f:\n-        return json.dump(data, f, **json_prettyprint_args)\n+    dumped_json = json_prettyprint(data=data)\n+    write_string(filename, dumped_json)\n \n \n def write_string(filename, data):\n-    with open(filename, \"w+\") as f:\n-        return f.write(data)\n+    \"\"\"\n+    Write a string to a file.\n+    Overwrite any data in that file.\n+\n+    We use an atomic write practice of creating a temporary file and then\n+    moving that temporary file to the given ``filename``. This prevents race\n+    conditions such as the file being read by another process after it is\n+    opened here but not yet written to.\n+\n+    It also prevents us from creating or truncating a file before we fail to\n+    write data to it because of low disk space.\n+\n+    If no file already exists at ``filename``, the new file is created with\n+    permissions 0o644.\n+    \"\"\"\n+    prefix = os.path.basename(filename)\n+    tmp_file_dir = os.path.dirname(os.path.realpath(filename))\n+    fd, temporary_filename = tempfile.mkstemp(prefix=prefix, dir=tmp_file_dir)\n+\n+    try:\n+        permissions = os.stat(filename).st_mode\n+    except FileNotFoundError:\n+        permissions = 0o644","path":"pkgpanda/util.py","position":67,"original_position":67,"commit_id":"4247045fbc4aaa0e612f839515698d0c56d89fd1","original_commit_id":"4247045fbc4aaa0e612f839515698d0c56d89fd1","user":{"login":"mhrabovcin","id":81354,"avatar_url":"https://avatars2.githubusercontent.com/u/81354?v=4","gravatar_id":"","url":"https://api.github.com/users/mhrabovcin","html_url":"https://github.com/mhrabovcin","followers_url":"https://api.github.com/users/mhrabovcin/followers","following_url":"https://api.github.com/users/mhrabovcin/following{/other_user}","gists_url":"https://api.github.com/users/mhrabovcin/gists{/gist_id}","starred_url":"https://api.github.com/users/mhrabovcin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mhrabovcin/subscriptions","organizations_url":"https://api.github.com/users/mhrabovcin/orgs","repos_url":"https://api.github.com/users/mhrabovcin/repos","events_url":"https://api.github.com/users/mhrabovcin/events{/privacy}","received_events_url":"https://api.github.com/users/mhrabovcin/received_events","type":"User","site_admin":false},"body":"I personally prefer `0o644` as it is standard form for linux commands. ","created_at":"2018-04-19T12:45:51Z","updated_at":"2018-04-19T12:45:51Z","html_url":"https://github.com/dcos/dcos/pull/2730#discussion_r182732450","pull_request_url":"https://api.github.com/repos/dcos/dcos/pulls/2730","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/dcos/dcos/pulls/comments/182732450"},"html":{"href":"https://github.com/dcos/dcos/pull/2730#discussion_r182732450"},"pull_request":{"href":"https://api.github.com/repos/dcos/dcos/pulls/2730"}},"in_reply_to_id":182730067},"pull_request":{"url":"https://api.github.com/repos/dcos/dcos/pulls/2730","id":180907081,"html_url":"https://github.com/dcos/dcos/pull/2730","diff_url":"https://github.com/dcos/dcos/pull/2730.diff","patch_url":"https://github.com/dcos/dcos/pull/2730.patch","issue_url":"https://api.github.com/repos/dcos/dcos/issues/2730","number":2730,"state":"open","locked":false,"title":"[DCOS-14199] Write strings to files atomically in pkgpanda.util","user":{"login":"adamtheturtle","id":797801,"avatar_url":"https://avatars3.githubusercontent.com/u/797801?v=4","gravatar_id":"","url":"https://api.github.com/users/adamtheturtle","html_url":"https://github.com/adamtheturtle","followers_url":"https://api.github.com/users/adamtheturtle/followers","following_url":"https://api.github.com/users/adamtheturtle/following{/other_user}","gists_url":"https://api.github.com/users/adamtheturtle/gists{/gist_id}","starred_url":"https://api.github.com/users/adamtheturtle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adamtheturtle/subscriptions","organizations_url":"https://api.github.com/users/adamtheturtle/orgs","repos_url":"https://api.github.com/users/adamtheturtle/repos","events_url":"https://api.github.com/users/adamtheturtle/events{/privacy}","received_events_url":"https://api.github.com/users/adamtheturtle/received_events","type":"User","site_admin":false},"body":"## High-level description\r\n\r\nThis changes `pkgpanda.util.write_string` to avoid a race condition as described in [DCOS-14199](https://jira.mesosphere.com/browse/DCOS-14199).\r\nI have also applied this change to some related functions as a precaution.\r\nI have stopped returning data from the functions as it appears that those return values are never used.\r\n\r\nThis also adds a test for `write_string` to enshrine the permission requirements and basic functionality of `write_string` (thanks @mhrabovcin for help). That test passes on `master` *and* with the changes here.\r\n\r\n## Corresponding DC/OS tickets (obligatory)\r\n\r\nThese DC/OS JIRA ticket(s) must be updated (ideally closed) in the moment this PR lands:\r\n\r\n  - [DCOS-14199](https://jira.mesosphere.com/browse/DCOS-14199) exhibitor.py try_shortcut() failed due to an empty file\r\n\r\n## Checklist for all PRs\r\n\r\n  - [ ] Added a comprehensible changelog entry to `CHANGES.md` or explain why this is not a user-facing change: See https://github.com/dcos/dcos/pull/2730#issuecomment-380506106.\r\n  - [ ] Included a test which will fail if code is reverted but test is not. If there is no test please explain here: This code is already called by the test suite. We could test the race condition with something like the following with a timeout but I'd rather not. I'm happy to discuss this with a reviewer:\r\n\r\n```python\r\n# This will trigger the error from the issue if run by two processes in parallel.\r\n# Calling the new function in the loop will not.\r\nfilename = 'foo.txt'\r\nwhile True:\r\n    with open(filename, 'w+') as f:\r\n        f.write('hello')\r\n\r\n    with open(filename) as f:\r\n        stripped = f.read().strip()\r\n        assert stripped != ''\r\n```\r\n\r\n  - [X] Read the [DC/OS contributing guidelines](https://github.com/dcos/dcos/blob/master/contributing.md)\r\n  - [X] Followed relevant code rules [Rules for Packages and Systemd](https://github.com/dcos/dcos/tree/master/docs)","created_at":"2018-04-11T13:39:38Z","updated_at":"2018-04-19T12:45:51Z","closed_at":null,"merged_at":null,"merge_commit_sha":"54ea7547fcae5ef055854d5b4882ef4bd29ea3d4","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[{"id":364878184,"url":"https://api.github.com/repos/dcos/dcos/labels/Ready%20For%20Review","name":"Ready For Review","color":"0e8a16","default":false}],"milestone":null,"commits_url":"https://api.github.com/repos/dcos/dcos/pulls/2730/commits","review_comments_url":"https://api.github.com/repos/dcos/dcos/pulls/2730/comments","review_comment_url":"https://api.github.com/repos/dcos/dcos/pulls/comments{/number}","comments_url":"https://api.github.com/repos/dcos/dcos/issues/2730/comments","statuses_url":"https://api.github.com/repos/dcos/dcos/statuses/4247045fbc4aaa0e612f839515698d0c56d89fd1","head":{"label":"adamtheturtle:adamdangoor/fix-try-shortcut-DCOS-14199","ref":"adamdangoor/fix-try-shortcut-DCOS-14199","sha":"4247045fbc4aaa0e612f839515698d0c56d89fd1","user":{"login":"adamtheturtle","id":797801,"avatar_url":"https://avatars3.githubusercontent.com/u/797801?v=4","gravatar_id":"","url":"https://api.github.com/users/adamtheturtle","html_url":"https://github.com/adamtheturtle","followers_url":"https://api.github.com/users/adamtheturtle/followers","following_url":"https://api.github.com/users/adamtheturtle/following{/other_user}","gists_url":"https://api.github.com/users/adamtheturtle/gists{/gist_id}","starred_url":"https://api.github.com/users/adamtheturtle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adamtheturtle/subscriptions","organizations_url":"https://api.github.com/users/adamtheturtle/orgs","repos_url":"https://api.github.com/users/adamtheturtle/repos","events_url":"https://api.github.com/users/adamtheturtle/events{/privacy}","received_events_url":"https://api.github.com/users/adamtheturtle/received_events","type":"User","site_admin":false},"repo":{"id":87240336,"name":"dcos","full_name":"adamtheturtle/dcos","owner":{"login":"adamtheturtle","id":797801,"avatar_url":"https://avatars3.githubusercontent.com/u/797801?v=4","gravatar_id":"","url":"https://api.github.com/users/adamtheturtle","html_url":"https://github.com/adamtheturtle","followers_url":"https://api.github.com/users/adamtheturtle/followers","following_url":"https://api.github.com/users/adamtheturtle/following{/other_user}","gists_url":"https://api.github.com/users/adamtheturtle/gists{/gist_id}","starred_url":"https://api.github.com/users/adamtheturtle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adamtheturtle/subscriptions","organizations_url":"https://api.github.com/users/adamtheturtle/orgs","repos_url":"https://api.github.com/users/adamtheturtle/repos","events_url":"https://api.github.com/users/adamtheturtle/events{/privacy}","received_events_url":"https://api.github.com/users/adamtheturtle/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/adamtheturtle/dcos","description":"DC/OS Build and Release tools","fork":true,"url":"https://api.github.com/repos/adamtheturtle/dcos","forks_url":"https://api.github.com/repos/adamtheturtle/dcos/forks","keys_url":"https://api.github.com/repos/adamtheturtle/dcos/keys{/key_id}","collaborators_url":"https://api.github.com/repos/adamtheturtle/dcos/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/adamtheturtle/dcos/teams","hooks_url":"https://api.github.com/repos/adamtheturtle/dcos/hooks","issue_events_url":"https://api.github.com/repos/adamtheturtle/dcos/issues/events{/number}","events_url":"https://api.github.com/repos/adamtheturtle/dcos/events","assignees_url":"https://api.github.com/repos/adamtheturtle/dcos/assignees{/user}","branches_url":"https://api.github.com/repos/adamtheturtle/dcos/branches{/branch}","tags_url":"https://api.github.com/repos/adamtheturtle/dcos/tags","blobs_url":"https://api.github.com/repos/adamtheturtle/dcos/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/adamtheturtle/dcos/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/adamtheturtle/dcos/git/refs{/sha}","trees_url":"https://api.github.com/repos/adamtheturtle/dcos/git/trees{/sha}","statuses_url":"https://api.github.com/repos/adamtheturtle/dcos/statuses/{sha}","languages_url":"https://api.github.com/repos/adamtheturtle/dcos/languages","stargazers_url":"https://api.github.com/repos/adamtheturtle/dcos/stargazers","contributors_url":"https://api.github.com/repos/adamtheturtle/dcos/contributors","subscribers_url":"https://api.github.com/repos/adamtheturtle/dcos/subscribers","subscription_url":"https://api.github.com/repos/adamtheturtle/dcos/subscription","commits_url":"https://api.github.com/repos/adamtheturtle/dcos/commits{/sha}","git_commits_url":"https://api.github.com/repos/adamtheturtle/dcos/git/commits{/sha}","comments_url":"https://api.github.com/repos/adamtheturtle/dcos/comments{/number}","issue_comment_url":"https://api.github.com/repos/adamtheturtle/dcos/issues/comments{/number}","contents_url":"https://api.github.com/repos/adamtheturtle/dcos/contents/{+path}","compare_url":"https://api.github.com/repos/adamtheturtle/dcos/compare/{base}...{head}","merges_url":"https://api.github.com/repos/adamtheturtle/dcos/merges","archive_url":"https://api.github.com/repos/adamtheturtle/dcos/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/adamtheturtle/dcos/downloads","issues_url":"https://api.github.com/repos/adamtheturtle/dcos/issues{/number}","pulls_url":"https://api.github.com/repos/adamtheturtle/dcos/pulls{/number}","milestones_url":"https://api.github.com/repos/adamtheturtle/dcos/milestones{/number}","notifications_url":"https://api.github.com/repos/adamtheturtle/dcos/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/adamtheturtle/dcos/labels{/name}","releases_url":"https://api.github.com/repos/adamtheturtle/dcos/releases{/id}","deployments_url":"https://api.github.com/repos/adamtheturtle/dcos/deployments","created_at":"2017-04-04T22:16:15Z","updated_at":"2017-04-04T22:16:17Z","pushed_at":"2018-04-19T12:07:36Z","git_url":"git://github.com/adamtheturtle/dcos.git","ssh_url":"git@github.com:adamtheturtle/dcos.git","clone_url":"https://github.com/adamtheturtle/dcos.git","svn_url":"https://github.com/adamtheturtle/dcos","homepage":"https://dcos.io","size":5956,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0"},"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"dcos:master","ref":"master","sha":"78dd89f5548b257972228ba9f37bca59c6b0dde1","user":{"login":"dcos","id":17648048,"avatar_url":"https://avatars1.githubusercontent.com/u/17648048?v=4","gravatar_id":"","url":"https://api.github.com/users/dcos","html_url":"https://github.com/dcos","followers_url":"https://api.github.com/users/dcos/followers","following_url":"https://api.github.com/users/dcos/following{/other_user}","gists_url":"https://api.github.com/users/dcos/gists{/gist_id}","starred_url":"https://api.github.com/users/dcos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dcos/subscriptions","organizations_url":"https://api.github.com/users/dcos/orgs","repos_url":"https://api.github.com/users/dcos/repos","events_url":"https://api.github.com/users/dcos/events{/privacy}","received_events_url":"https://api.github.com/users/dcos/received_events","type":"Organization","site_admin":false},"repo":{"id":56184050,"name":"dcos","full_name":"dcos/dcos","owner":{"login":"dcos","id":17648048,"avatar_url":"https://avatars1.githubusercontent.com/u/17648048?v=4","gravatar_id":"","url":"https://api.github.com/users/dcos","html_url":"https://github.com/dcos","followers_url":"https://api.github.com/users/dcos/followers","following_url":"https://api.github.com/users/dcos/following{/other_user}","gists_url":"https://api.github.com/users/dcos/gists{/gist_id}","starred_url":"https://api.github.com/users/dcos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dcos/subscriptions","organizations_url":"https://api.github.com/users/dcos/orgs","repos_url":"https://api.github.com/users/dcos/repos","events_url":"https://api.github.com/users/dcos/events{/privacy}","received_events_url":"https://api.github.com/users/dcos/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/dcos/dcos","description":"DC/OS - The Datacenter Operating System ","fork":false,"url":"https://api.github.com/repos/dcos/dcos","forks_url":"https://api.github.com/repos/dcos/dcos/forks","keys_url":"https://api.github.com/repos/dcos/dcos/keys{/key_id}","collaborators_url":"https://api.github.com/repos/dcos/dcos/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/dcos/dcos/teams","hooks_url":"https://api.github.com/repos/dcos/dcos/hooks","issue_events_url":"https://api.github.com/repos/dcos/dcos/issues/events{/number}","events_url":"https://api.github.com/repos/dcos/dcos/events","assignees_url":"https://api.github.com/repos/dcos/dcos/assignees{/user}","branches_url":"https://api.github.com/repos/dcos/dcos/branches{/branch}","tags_url":"https://api.github.com/repos/dcos/dcos/tags","blobs_url":"https://api.github.com/repos/dcos/dcos/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/dcos/dcos/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/dcos/dcos/git/refs{/sha}","trees_url":"https://api.github.com/repos/dcos/dcos/git/trees{/sha}","statuses_url":"https://api.github.com/repos/dcos/dcos/statuses/{sha}","languages_url":"https://api.github.com/repos/dcos/dcos/languages","stargazers_url":"https://api.github.com/repos/dcos/dcos/stargazers","contributors_url":"https://api.github.com/repos/dcos/dcos/contributors","subscribers_url":"https://api.github.com/repos/dcos/dcos/subscribers","subscription_url":"https://api.github.com/repos/dcos/dcos/subscription","commits_url":"https://api.github.com/repos/dcos/dcos/commits{/sha}","git_commits_url":"https://api.github.com/repos/dcos/dcos/git/commits{/sha}","comments_url":"https://api.github.com/repos/dcos/dcos/comments{/number}","issue_comment_url":"https://api.github.com/repos/dcos/dcos/issues/comments{/number}","contents_url":"https://api.github.com/repos/dcos/dcos/contents/{+path}","compare_url":"https://api.github.com/repos/dcos/dcos/compare/{base}...{head}","merges_url":"https://api.github.com/repos/dcos/dcos/merges","archive_url":"https://api.github.com/repos/dcos/dcos/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/dcos/dcos/downloads","issues_url":"https://api.github.com/repos/dcos/dcos/issues{/number}","pulls_url":"https://api.github.com/repos/dcos/dcos/pulls{/number}","milestones_url":"https://api.github.com/repos/dcos/dcos/milestones{/number}","notifications_url":"https://api.github.com/repos/dcos/dcos/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/dcos/dcos/labels{/name}","releases_url":"https://api.github.com/repos/dcos/dcos/releases{/id}","deployments_url":"https://api.github.com/repos/dcos/dcos/deployments","created_at":"2016-04-13T20:31:58Z","updated_at":"2018-04-19T05:45:14Z","pushed_at":"2018-04-19T12:07:39Z","git_url":"git://github.com/dcos/dcos.git","ssh_url":"git@github.com:dcos/dcos.git","clone_url":"https://github.com/dcos/dcos.git","svn_url":"https://github.com/dcos/dcos","homepage":"https://dcos.io","size":6012,"stargazers_count":1940,"watchers_count":1940,"language":"Python","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":389,"mirror_url":null,"archived":false,"open_issues_count":120,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0"},"forks":389,"open_issues":120,"watchers":1940,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/dcos/dcos/pulls/2730"},"html":{"href":"https://github.com/dcos/dcos/pull/2730"},"issue":{"href":"https://api.github.com/repos/dcos/dcos/issues/2730"},"comments":{"href":"https://api.github.com/repos/dcos/dcos/issues/2730/comments"},"review_comments":{"href":"https://api.github.com/repos/dcos/dcos/pulls/2730/comments"},"review_comment":{"href":"https://api.github.com/repos/dcos/dcos/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/dcos/dcos/pulls/2730/commits"},"statuses":{"href":"https://api.github.com/repos/dcos/dcos/statuses/4247045fbc4aaa0e612f839515698d0c56d89fd1"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"mhrabovcin"}}