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

fix upgrade-script #1858

Closed
wants to merge 15 commits into from
Closed

fix upgrade-script #1858

wants to merge 15 commits into from

Conversation

sdetweil
Copy link
Collaborator

@sdetweil sdetweil commented Jan 2, 2020

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 2, 2020

this is a really a hotfix, need before 2.11, so that the upgrader works

@MichMich
Copy link
Collaborator

MichMich commented Jan 3, 2020

Before I release a hot fix (which I really try to avoid in this project due to all the update notifications), it might be worth to discuss this issue: #1860

(Edit:Fixed link to correct issue)

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 3, 2020

the script wants to make sure all installed and configured modules which have dependencies are also updated.. npm install for the base does not do this, and results in a dead mirror. This is the number 1 error on each update cycle.

now, how to determine the configured modules... i wrote quick js to load the config and loop thru the module array to check the disabled flag. most of us developers have many modules installed but only a few enabled.

except this new file is not present on the users system .. so the script downloads it if needed.

except, it is downloaded BEFORE the repo updates are done, and because the file is now included in the repo, we have a merge conflict. this change moves the download to just before running if not already present.

without this fix, the script will always fail.
the PR that include the script also updated the readme on the update process to use the script.
so the github site says use the script.

there is a second fix, about to push, when there are changed files, the user has the opportunity to save them (via git stash). except stash requires the users email and name. most users have never set that, and so stash will fail, and update will fail. (pull would have failed too due to the conflicts, and the user has no idea what to do).. i use a temporary name and email, just for these commands, but in two cases the $ variable identifier was left off. so the stash command still fails.

@MichMich MichMich mentioned this pull request Jan 3, 2020
10 tasks
@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 3, 2020

this build failed again for travis problems.. only a script file was changed... and weather tests failed again.. unrelated to my change

@MichMich
Copy link
Collaborator

MichMich commented Jan 3, 2020

Restarted Travis. Succeeded without issues.

@MichMich
Copy link
Collaborator

MichMich commented Jan 4, 2020

If we need to release this before 2.11, we might want to make a separate branch, so all other changes that are part of the 2.11 release won't be in the hot fix release. Do you want me to create a separate branch so we can release this one time hotfix?

@pgporada
Copy link

pgporada commented Jan 5, 2020

There's an unfixed bug that I think this PR can address. The variable $ logfile should be ${logfile} at https://github.com/MichMich/MagicMirror/pull/1858/files#diff-481c0639db88cf5d7ddd89304e0bf726R247

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2020

separate branch, I am ok with that

@pgporada
Copy link

pgporada commented Jan 5, 2020 via email

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2020

@pgporada thanks for the report../ I've added this fix to the separate script repo now too

https://github.com/sdetweil/MagicMirror_scripts

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2020

@MichMich as the original PR for these changes also changed the upgrade instructions in the readme, do you want to revert those back to the pull approach as well?

@MichMich
Copy link
Collaborator

MichMich commented Jan 5, 2020

Sam, just to be sure (before I go trough the process of making a new branch and preparing a 2.10.1 release): Is this change also necessary if we continue the work discussed in #1860? Does this save an issue for people currently upgrading? Or does this solve an issue for future upgrades?

Thanks for your patience!

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 5, 2020

The installer is also broken, so I think u need to do this.

@MichMich MichMich changed the base branch from develop to 2.10.1-hotfix January 5, 2020 21:24
@MichMich
Copy link
Collaborator

MichMich commented Jan 5, 2020

I created the branch based on the current master branch. This PR probably needs to be rebased so it doesn't include the previous commits made on the develop branch.

Thanks!

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 6, 2020

i think I want to change the focus of this hotfix.

if I change the readme, so that it points to my repo for automated install and update, etc, then
I can fix whatever, whenever. no code changes in the repo itself. and U can work towards #1860

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jan 6, 2020

I propose close this pr for #1865 instead. all future PR's against install/update rejected, redirected to external repo

@MichMich MichMich closed this Jan 6, 2020
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.

5 participants