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

Provide fresh auto updater in releases #2212

Merged
merged 2 commits into from
Dec 22, 2017

Conversation

HebaruSan
Copy link
Member

This pull request replaces #2204, which was closed because its approach wasn't appropriate. See that PR for details of the problem being solved.

New approach:

  • Add AutoUpdater.exe to the list of downloadable files when creating a new release on GitHub
  • Update client to look for this file and use it, falling back to the old URL if it's missing

This change assumes that the list of assets in the release will be in the same order as they are in the .travis.yml file. I believe this would be the case because it's an array rather than a dictionary, and the easiest way for Travis to implement it would be a simple foreach loop, but I was not able to find documentation or a concrete example to confirm this.

We should mention in the next release notes that users do not need to download AutoUpdater.exe manually.

Partially fixes #2140. The error will still occur when updating to this release, because the old clients will still use the 2-year-old auto updater exe, but updating from this release to the next release will use the latest auto update code. If we publish one last manual release to https://github.com/KSP-CKAN/CKAN-autoupdate/releases , then the current clients will update properly as well.

@HebaruSan HebaruSan added AutoUpdate Issues affecting the automatic updating Bug Something is not working as intended Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...) Pull request labels Dec 9, 2017
@HebaruSan HebaruSan force-pushed the fix/release-auto-updater branch 2 times, most recently from ed85977 to cea39d0 Compare December 21, 2017 05:17
fetchedCkanUrl = RetrieveUrl(response, 0);
// Check whether the release includes the auto updater
if (response.assets.Count >= 2) {
// Second asset is AutoUpdater.exe
Copy link
Member

Choose a reason for hiding this comment

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

The CKAN.app is now the second asset, and aren't we going to add in a .deb as well? Can we use a regex on the assets to find the correct one, rather than hard-code the position?

Copy link
Member Author

@HebaruSan HebaruSan Dec 21, 2017

Choose a reason for hiding this comment

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

Yup, this needs to be updated every time we add to that list (which hopefully won't be too many more times). It was 1 before the mac PR got merged, and I'll be incrementing it again now that the deb project is in. Luckily GitHub helpfully notices that these PRs all conflict with one another, which ensures that we don't merge without reconciling them.

Though yeah, you're right, it should examine the assets rather than hard coding an index. Once this client version is out in the wild, it would be impossible to rearrange things...

- Add AutoUpdater.exe to release file list in travis-ci
- Look for auto updater file in main release
- Update tests now that releases may contain multiple files
@HebaruSan HebaruSan force-pushed the fix/release-auto-updater branch from cea39d0 to 19328d4 Compare December 21, 2017 22:44
@HebaruSan
Copy link
Member Author

After the latest commit, it now checks the URLs to find the auto updater rather than assuming a particular index.

@politas politas merged commit 050acdb into KSP-CKAN:master Dec 22, 2017
politas added a commit that referenced this pull request Dec 22, 2017
@politas politas removed Bug Something is not working as intended Pull request labels Dec 22, 2017
@HebaruSan HebaruSan deleted the fix/release-auto-updater branch December 22, 2017 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoUpdate Issues affecting the automatic updating Infrastructure Issues affecting everything around CKAN (the GitHub repos, build process, CI, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updater can't deal with spaces in path
2 participants