Simplify download link logic #953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-953
36degrees
force-pushed
the
simplify-download-logic
branch
from
947382d to
8e7a990
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-953
36degrees
force-pushed
the
simplify-download-logic
branch
from
8e7a990 to
dbbd14a
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-953
|
as commented on Slack so just to keep a record :) we might want to keep |
|
A comment on why we had the seeming complex logic to look up the url - added in #244 I'm pretty sure it's because the same kit is used for docs and possibly self hosting. If the link were derived using the number in the Github (at the time) didn't have a way of linking to something like |
govuk-design-system-ci
temporarily deployed
to
govuk-prototype-kit-pr-953
At the minute the tests hang when running on macOS:
> Jest did not exit one second after the test run has completed.
>
> This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
If you run `jest —detectOpenHandles` this seems to suggest the issue is with sync-request:
```
Jest has detected the following 1 open handle potentially keeping Jest from exiting:
● PROCESSWRAP
at start (node_modules/sync-rpc/lib/index.js:33:13)
at sendMessage (node_modules/sync-rpc/lib/index.js:133:17)
at createClient (node_modules/sync-rpc/lib/index.js:173:27)
at Object.<anonymous> (node_modules/sync-request/lib/index.js:16:14)
```
`sync-request` is only used to fetch the latest version of the kit from the GitHub API. We currently have to do this becuase, although GitHub provides a `/latest/` endpoint to link to the latest release [1], you can only link to assets if you know the asset filename, and the asset filenames are currently based on the version numbers and so not predictable unless you know the latest version number.
However, we can avoid calling the API at all by instead using the version number as defined in the package.json file. This allows us to simplify the code and remove the sync-request library entirely.
This does rely on the most recent release having been tagged (and thus the release created in GitHub) before the equivalent version is deployed to Heroku. As we have ‘Wait for CI to pass before deploy’ enabled in the Heroku settings I believe this should be the case.
[1]: https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/linking-to-releases#:~:text=To%20link%20directly%20to%20a%20download%20of%20your%20latest%20release%20asset%2C%20link%20to%20%2Fowner%2Fname%2Freleases%2Flatest%2Fdownload%2Fasset-name.zip
Now that we’re relying on the version defined in package.json, it will only ever link to the same version as used by the copy of the kit, not the actual latest version. This is fine for the docs site on Heroku, which should always be running the latest version, but could cause issues if someone tries to download the latest version by using the docs running locally in their kit. When the kit is not running in ‘promo mode’ (as used by the live docs site), just point the user to the latest release page on GitHub.
36degrees
force-pushed
the
simplify-download-logic
branch
from
1113292 to
26f6746
Compare
|
I've pushed another commit to fall back to just pointing to https://github.com/alphagov/govuk-prototype-kit/releases/latest when not in 'promo mode' – so the deployed docs site should continue to point directly to the zip, but in the unlikely situation that a user follows the link from their local docs they will end up on the GitHub page for the latest release. |
joelanman
approved these changes
Nov 2, 2020
|
Created an issue against the v10.0 milestone to remove |
hannalaakso
approved these changes
Nov 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the minute the tests hang when running on macOS:
If you run
jest —detectOpenHandlesthis seems to suggest the issue is with sync-request:sync-requestis only used to fetch the latest version of the kit from the GitHub API. We currently have to do this becuase, although GitHub provides a/latest/endpoint to link to the latest release, you can only link to assets if you know the asset filename, and the asset filenames are currently based on the version numbers and so not predictable unless you know the latest version number.However, we can avoid calling the API at all by instead using the version number as defined in the package.json file. This allows us to simplify the code and remove the sync-request library entirely.
This does rely on the most recent release having been tagged (and thus the release created in GitHub) before the equivalent version is deployed to Heroku. As we have ‘Wait for CI to pass before deploy’ enabled in the Heroku settings I believe this should be the case.