Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change JSON key "last_updated" in sdw-update-flag to "last_status_update" #433

Merged
merged 4 commits into from
Jan 30, 2020

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Jan 28, 2020

The use of last_updated here is ambiguous with with the usage in the separate file sdw-last-udpated, which contains the time of the last successful update of the SecureDrop Workstation.

The last_updated key in sdw-last-updated contains the timestamp of the last time the status itself was written to disk, regardless of success or failure.

I have also renamed the file itself to sdw-update-status for clarity, since that is the purpose of the file.

Status

Ready for review

Testing

  • Tests should still pass
  • Verify that the status file is written as expected to sdw-update-status when running the updater
  • Verify that make clean removes file under old name

…pdate"

The use of `last_updated` here is ambiguous with with the usage in the
separate file `sdw-last-udpated`, which contains the time of the
last successful update of the SecureDrop Workstation.

The `last_updated` key in `sdw-last-updated` contains the timestamp of
the last time the status itself was written to disk, regardless of
success or failure.
@eloquence eloquence force-pushed the flag-modify-update-oh-my branch from e222458 to 8efc122 Compare January 28, 2020 08:08
@eloquence
Copy link
Member Author

Personally I think sdw-update-status would be a more self-explanatory name for the flag file, unless we're future-proofing it in some way by calling it "flag", and I'd be happy to rename it if that sounds reasonable (it's not used for anything yet), but for now focused on just addressing the dueling use of last_updated which I found very confusing, and which @emkll agreed would be reasonable to tweak.

("flag" doesn't clearly speak to the purpose of the file and
is not used in the JSON keys)
@eloquence
Copy link
Member Author

Per brief chat at standup, renamed the file as well; leaving in draft for now until I give it one quick run-through in Qubes.

@eloquence eloquence marked this pull request as ready for review January 29, 2020 02:29
@eloquence
Copy link
Member Author

Tested in Qubes, works as expected.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff and functional testing looks good to me.
The only issue that should be addressed before merge, IMO, should be a clean up of all the dom0 flags (see inline).

While testing this pr, I discovered an related but not related bug. If you think it makes sense to take a stab at fixing this, that would be great otherwise I will open a follow up.

The reboot_required case should be handled at this point and show the reboot button

.

str:

  • apply updates that require a reboot
  • close app
  • open app (no updates required), error is shown (because REBOOT_REQUIRED case is not handled, and is part of the catch-all)

@@ -21,6 +21,7 @@ remove-dom0-sdw-config-files:
- /etc/qubes-rpc/policy/securedrop.Log
- /etc/qubes-rpc/policy/securedrop.Proxy
- /home/{{ gui_user }}/Desktop/securedrop-launcher.desktop
- /home/{{ gui_user }}/.securedrop_launcher/sdw-update-flag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least remove the other flags as well (the current, working flags) and consider deleting the entire folder (which include the logs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about that; I thought you maybe intentionally omitted the folder. But I agree, it should clean up everything.

Copy link
Member Author

@eloquence eloquence Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and tested. If you prefer to preserve the logs we can list individual files instead, but it would feel a bit wrong to me to leave this directory hanging around after a make clean.

@eloquence
Copy link
Member Author

The reboot_required case should be handled at this point and show the reboot button

I'll file a separate issue for that, thanks for the clear STR

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @eloquence , lgtm

@emkll emkll merged commit 0d517a5 into master Jan 30, 2020
@emkll emkll deleted the flag-modify-update-oh-my branch January 30, 2020 14:40
cfm pushed a commit that referenced this pull request Apr 1, 2024
Change JSON key "last_updated" in `sdw-update-flag` to "last_status_update"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants