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

Installer: identity directory defaults to %APPDATA%\Storj\Identity\Storagenode #3216

Merged
merged 8 commits into from
Oct 10, 2019

Conversation

Fadila82
Copy link
Contributor

@Fadila82 Fadila82 commented Oct 8, 2019

What: default value in the Identity Folder dialog of the Windows Installer points to %APPDATA%\Storj\Identity\Storagenode

Why: because it is the default location for identity tool to generate new storage node identity. Currently, the default is set to the root of the largest drive, which requires the user to always navigate to the location of their identity.

Acceptance criteria:

  • The Identity Folder dialog displays %APPDATA%\Storj\Identity\Storagenode as the default value, where %APPDATA% is expanded to the actual location for the current user, e.g. C:\Users\IEUser\AppData\Roaming\Storj\Identity\Storagenode.

Please describe the tests: no test
Please describe the performance impact: none

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@Fadila82 Fadila82 added the Request Code Review Code review requested label Oct 8, 2019
@Fadila82 Fadila82 requested a review from a team October 8, 2019 23:00
@cla-bot cla-bot bot added the cla-signed label Oct 8, 2019
@ghost ghost requested review from JessicaGreben and mniewrzal and removed request for a team October 8, 2019 23:00
<DirectorySearch Id='search1' Path='[PersonalFolder]' />
<DirectorySearch Id='search2' Path='[AppDataFolder]\Storj\Identity\storagenode'/>
</Property>
<SetDirectory Action="SetIdentityDir" Id="IDENTITYDIR" Value="[STORJ_DEFAULTIDENTITYDIR]" Sequence="first">STORJ_DEFAULTIDENTITYDIR</SetDirectory>
Copy link
Member

Choose a reason for hiding this comment

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

I did more testing and I found that only this statement is enough to replace the above 5 lines:

<SetDirectory Action="SetIdentityDir" Id="IDENTITYDIR" Value="[AppDataFolder]Storj\Identity\Storagenode" Sequence="first">NOT Installed</SetDirectory>

Not that:

  • The previous time I suggested this, I missed the Sequence="first" argument (that you correctly found that must be used). Without it, the IDENTITYDIR was reset to the default value when the Install button was clicked, although the user may have selected another value in the dialog.
  • We don't need the \ after [AppDataFolder] as its value already ends with \.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DirectorySearch lines are meant to make sure we open an existing directory. If the default identity files location does not exist, personal folder is used as a fallback. But maybe that's not what we want?

Copy link
Member

Choose a reason for hiding this comment

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

Oh!!! This is so good. I had no idea we can actually do it. Forget what I am saying then :)

<Property Id="STORJ_STORAGEDIR">STORAGEDIR</Property>
<Property Id="STORJ_STORAGE">1.0</Property>
<Property Id="STORJ_BANDWIDTH">2.0</Property>
<Property Id="STORJ_IDENTITYDIR">IDENTITYDIR</Property>
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's move this back to its original location - between the INSTALLFOLDER and STORAGEDIR properties.

installer/windows/Product.wxs Outdated Show resolved Hide resolved
kaloyan-raev
kaloyan-raev previously approved these changes Oct 10, 2019
@Fadila82 Fadila82 merged commit cf6f8fb into master Oct 10, 2019
@Fadila82 Fadila82 deleted the orange/win-installer-default-identity-dir branch October 10, 2019 13:51
littleskunk pushed a commit that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants