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

SDA-4514: NSIS Migration #2142

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

NguyenTranHoangSym
Copy link
Contributor

@NguyenTranHoangSym NguyenTranHoangSym commented May 3, 2024

Description

Adding new NSIS script for migration of config file from 23.1 upto day
this script will eliminate duplicated config

@NguyenTranHoangSym NguyenTranHoangSym self-assigned this May 13, 2024
@NguyenTranHoangSym NguyenTranHoangSym marked this pull request as ready for review May 13, 2024 03:28
@NguyenTranHoangSym NguyenTranHoangSym changed the title SDA-4514: Test NSIS SDA-4514: NSIS Migration May 13, 2024
push latestAutoUpdateChannelEnabled
call SearchAndReplace
FunctionEnd

!macro copySystemGlobalConfig
Copy link
Member

Choose a reason for hiding this comment

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

This logic also needs to be applied for per user install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the logic to per user

build/installer.nsh Show resolved Hide resolved
build/installer.nsh Show resolved Hide resolved
@@ -56,6 +244,12 @@ FunctionEnd
!insertmacro UAC_RunElevated
Quit
${endif}
call updateConfigIsPodUrlEditable
call updateConfigForceAutoUpdate
Copy link
Contributor

Choose a reason for hiding this comment

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

indent issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep indentation I dont have any format available

@@ -56,6 +244,12 @@ FunctionEnd
!insertmacro UAC_RunElevated
Quit
${endif}
call updateConfigIsPodUrlEditable
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we update config file for global install only?

Copy link
Member

@KiranNiranjan KiranNiranjan left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 remove comments

Comment on lines 152 to 161
#FileOpen $8 "E:\Projects\NSIS\text.txt" w
#FileWrite $8 "$0$\r$\n"
#FileWrite $8 "$1$\r$\n"
#FileWrite $8 "$4$\r$\n"
#FileClose $8

#LogText $0
#DetailPrint $0
#DetailPrint $1
#DetailPrint $4
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

Comment on lines 115 to 118
#FileWrite $8 "At done"
#FileWrite $8 "$4$\r$\n"
#FileWrite $8 "At done"
#FileWrite $8 "$R4$\r$\n"
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

@@ -44,6 +242,12 @@ FunctionEnd
!macroend

!macro perUserM
call updateConfigIsPodUrlEditable
call updateConfigForceAutoUpdate
Copy link
Contributor

Choose a reason for hiding this comment

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

indent issue

!macroend

Function SearchAndReplace
${If} $PerUser == "exists"
Copy link
Contributor

Choose a reason for hiding this comment

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

indent issue here too

@@ -44,6 +242,12 @@ FunctionEnd
!macroend

!macro perUserM
call updateConfigIsPodUrlEditable
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer if we have all call in one function which can be called in perUserM and allUserM macros

@NguyenTranHoangSym NguyenTranHoangSym merged commit 47bbaf9 into finos:main May 16, 2024
6 checks passed
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.

3 participants