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

162 windows installer setup #275

Merged
merged 38 commits into from
Dec 6, 2023
Merged

162 windows installer setup #275

merged 38 commits into from
Dec 6, 2023

Conversation

raviSussol
Copy link
Collaborator

@raviSussol raviSussol commented Dec 4, 2023

Closes #162

πŸ‘©πŸ»β€πŸ’» What does this PR do?

Added a windows installer build script for the notify app.

πŸ§ͺ How has/should this change been tested?

Download installer from here and test it.
Or, create a new tag (starts with vxx.xx.xx) and then push to the origin. Jenkins will automatically fire the windows installer build job in the https://jenkins.msupply.org:8443/job/notifyMain/

πŸ’Œ Any notes for the reviewer?

πŸ“ƒ Documentation

No user facing changes

or

  • Functional change 1
  • Functional change 2

Copy link

github-actions bot commented Dec 4, 2023

Bundle size difference

Comparing this PR to main

Old size New size Diff
5.2 MB 5.2 MB 0 B (0.00%)

raviSussol and others added 3 commits December 4, 2023 16:27
Update license to reflect opensource
Fix formatting issues in license file
Copy link
Collaborator

@jmbrunskill jmbrunskill left a comment

Choose a reason for hiding this comment

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

Great work!

However, It looks like the templates directory is missing?
Can you add this, and make sure it gets updated with an upgrade?
This is needed for the coldchain alerts, they use these templates.

image

Is it possible to get the build number from the git tag here?
image

I think we should probably use Notify instead of Notify Service (it's already a service)
image

Similar for the install location, I think we should probably use C:\Program Files\notify?
That does potentially create a problem for existing installs, but we can be across that when we update from the manual install to this installer...

build/windows/notify_service.suf Outdated Show resolved Hide resolved
@jmbrunskill
Copy link
Collaborator

jmbrunskill commented Dec 6, 2023

I also notice that if we're upgrading we still get the same prompts for passwords etc. Even though these are already in the config file?
image

Is it possible to only ask these questions on the first install?

@raviSussol
Copy link
Collaborator Author

Thanks for reviewing @jmbrunskill. I've updated a pr - addressing all your mentioned issues above. Could you please review again? The new installer: https://jenkins.msupply.org:8443/job/notifyMain%20-%20Installers/ for testing.

Copy link
Collaborator

@jmbrunskill jmbrunskill left a comment

Choose a reason for hiding this comment

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

Thanks for the fast work Ravi!
Working great!

@jmbrunskill jmbrunskill merged commit 4d5709f into main Dec 6, 2023
@jmbrunskill jmbrunskill deleted the 162-windows-installer-setup branch December 6, 2023 20:33
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.

Windows installer
2 participants