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

Update config.go to include default pinnedPackages #36

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

kalgen432
Copy link
Contributor

@kalgen432 kalgen432 commented Sep 11, 2023

pinnedPackages now defaults to the 4 necessary to make launcher work, per #34. Pushing this and triggering a new build of screeps-launcher also seems to address the failures to download yarn from github and address #37 too.

pinnedPackages:
ssri: 8.0.1
cacache: 16.1.3
passport-steam: 1.0.17
minipass-fetch: 3.0.3

@kalgen432
Copy link
Contributor Author

If it helps to test it, I pushed a rebuilt image with this PR: kalgen432/screeps-launcher.

@AlinaNova21
Copy link
Member

I'm not a fan of hardcoding this, seems like it could potentially cause issues when the packages and requirements shift in the future, it might be worth adding a note and link to the issue in the README though.

@kalgen432
Copy link
Contributor Author

kalgen432 commented Sep 12, 2023

Thanks for taking a look.

I don't know if this is what you meant by hardcoding, but I think this PR specifies those packages as a default, and any pinnedPackages: section of their config would override it? That is, adding this PR doesn't introduce any new issues or remove flexibility.

For first-time users:
With this PR, screeps-launcher will work with any previously published config out of the box. Without this PR, those configs are broken unless updated with a pinnedPackages section.

For longer-term users who have an old pull and and old volume that's already built, it should keep working both with and without this PR.

For longer-term users who have an old pull and are trying to build a new volume, regardless of this PR they have to do work to fix it. Re-pulling the image is enough if we use this PR and keep the image defaults updated with current pins, otherwise adding a pinnedPackages would also work regardless of this PR (with or without it).

I'd tend to prefer both updating the defaults to be working and updating the README to leave more breadcrumbs for future breaks to suggest re-pulling the image and updating pinnedPackages. But if you don't agree, I'll abandon this and prep a PR for just the README updates.

@AlinaNova21
Copy link
Member

Hmm, good points, I guess the README can be updated to mention it and this could be merged for now.

@AlinaNova21 AlinaNova21 merged commit d0b47e6 into screepers:master Sep 13, 2023
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.

2 participants