Closed chris-gds closed 3 years ago
Thanks for turning this into an issue @chris-gds
Viewing this app for the first time and starting up in isolation (to resolve an a11y issue) it's not clear that the typical Frontend startup process outlined in the docs does not apply to this app.
By this process do you mean: https://docs.publishing.service.gov.uk/manual/local-frontend-development.html ? and if so I'm not sure why it didn't apply as I think it should?
I think the thing that is a bit fishy is that very few GOV.UK app readme's have a "Running the application" the application section, and the lack of one suggests that someone forgot to add it, rather than it purposely isn't there. Indeed, it was only recently removed as the information was out of date.
If we're able to reach a resolution on this issue, it'd be good if it was something that could apply across various GOV.UK Docker apps so we'd want something simple that could be repeated.
Personally I'd be happy with something like:
### Running the application
This application is designed to be run within the [GOV.UK Docker](https://github.com/alphagov/govuk-docker) development environment. You can also [run this application in isolation](https://docs.publishing.service.gov.uk/manual/local-frontend-development.html#using-startup-scripts) for frontend development.
I'd want to keep anything we put brief to reduce the risks of information falling out of date.
@benthorner, as someone involved in the previous removal, what are your thoughts?
Thanks for tagging me @kevindew.
@chris-gds I'm nor sure I understand the problem. Could you help me understand what's wrong?
By this process do you mean: https://docs.publishing.service.gov.uk/manual/local-frontend-development.html?
Yes
and if so I'm not sure why it didn't apply as I think it should?
This is because the docs within this link read:
If you are making changes to a frontend app and nothing else, you can view these changes by running the application's
./startup.sh
script.
This is not applicable to this application to run in isolation, even if you wish to only update the frontend you have to use Docker --live
.
Indeed, it was only recently removed as the information was out of date.
This makes sense as ./startup.sh
does not apply to this app, only govuk-docker up email-alert-frontend-app-live
realistically applies to work on this app
Personally I'd be happy with something like:
### Running the application
This application is designed to be run within the [GOV.UK Docker](https://github.com/alphagov/govuk-docker) development environment. You can also [run this application in isolation](https://docs.publishing.service.gov.uk/manual/local-frontend-development.html#using-startup-scripts) for frontend development.
It's pretty close, however I wouldn't say the last sentence is accurate as it could be. By clicking this link a user could follow the startup.sh
process to run this application in isolation as this is listed first and does not apply to this app.
In order to correctly make changes to this app's Frontend I would have found something like this helpful:
### Running the application
This application is designed to be run within the [GOV.UK Docker](https://github.com/alphagov/govuk-docker) development environment using the `--live` flag.
I reference the --live
as this documentation suggest several different methods
My ideal update would be something like:
### Running the application
This application is designed to be run within the [GOV.UK Docker](https://github.com/alphagov/govuk-docker) development environment.
`govuk-docker up email-alert-frontend-app-live`
## Troubleshooting
(`startup.sh` does not apply to this app)
@benthorner
As part of this PR there was a request to update the README
to ensure the guidance around starting up this app in isolation from a Frontend POV was clearer and faster. It was discovered there are some nuances to this particular app which means that more global documentation does not apply quite the same.
It was decided this might be a larger topic and to extract out the README
update into an issue to reach a consensus.
@chris-gds thanks for the handy summary! I understand the problem now. We should bear in mind that the "live" way of running apps is just a convenience. In theory, any developer can also have a local copy of data and just develop against that. So we shouldn't be too prescriptive about how to run a given app. I use a mixture of both, for example.
My preference is that we should avoid repeating the same instructions in every app, and refer to the usage guidance in GOV.UK Docker. While it's longer, it saves time versus maintaining different/redundant instructions for each app.
### Running the app
See the usage instructions in [GOV.UK Docker](https://github.com/alphagov/govuk-docker#usage).
It also sounds like we can also delete the startup.sh
script for this app, since GOV.UK Docker doesn't rely on them (it's something of a smell that so many other apps still have them).
I guess there's still some ambiguity if someone is coming from the dev doc. We could fix this by adding "these instructions apply to any frontend app...with a startup.sh
script".
Would that have been enough to help you at the time?
I understand the problem now. We should bear in mind that the "live" way of running apps is just a convenience. In theory, any developer can also have a local copy of data and just develop against that.
I'm not sure a local copy of the data is always required or even desirable (eg in this context and to run in isolation)
So we shouldn't be too prescriptive about how to run a given app. I use a mixture of both, for example.
Me too, I partially agree - you should be free to choose the startup method that suits, however in this context I think the number of choices is reduced. (Possibly to one? Can you start up this app in isolation another way?)
My preference is that we should avoid repeating the same instructions in every app, and refer to the usage guidance in GOV.UK Docker. While it's longer, it saves time versus maintaining different/redundant instructions for each app.
Would this apply to the test snippet as well? $ bundle exec rake
could this be moved to more global documentation?
It also sounds like we can also delete the startup.sh script for this app, since GOV.UK Docker doesn't rely on them (it's something of a smell that so many other apps still have them).
Potentially this could resolved the opposite way with ./startup.sh --live
working as expected within this app? I think from the previous conversation and the comment, that this is classed as a smell, that this method is wished to be phrased out long-term..
I guess there's still some ambiguity if someone is coming from the dev doc. We could fix this by adding "these instructions apply to any frontend app...with a startup.sh script".
I think it's better. I'm not sure it's ideal from a Content POV - I'm wondering how many FE apps can't be used with ./startup
- is it just this one?
Would that have been enough to help you at the time?
I'm thinking about the next person that wishes to start this repo in isolation to work on the FE only. How can the doc be updated to cater for difference Dev audiences with different goals? My UX preference would be a working snippet in the README
that runs in this app in isolation much like the testing snippet.
If this isn't possible, then perhaps as a second choice - updating this doc to reference that Docker (& --live
) is required and startup.sh
doesn't apply to this app via the method suggested
So, as I understand it the problem is that ./startup.sh
doesn't work for this app and the documentation states that the startup script works for all apps. I'd like to scope this to email-alert-frontend only.
There's a discussion about removing the documentation for the startup.sh
script which is interesting.
I think that most of the problems highlighted in this issue could be solved by shifting the order of the documentation, with Docker listed before startup.sh; that way people would pick that up first.
As well, the startup script documentation should have a caveat added saying that this might not work with certain apps, including email alert frontend. As we know about more apps, we can update the documentation.
I think that this would help point developers in the correct directions without being too much work - and I'm happy to raise a pull request to do so.
So, as I understand it the problem is that ./startup.sh doesn't work for this app
Exactly, you can't move through it / render certain pages.
and the documentation states that the startup script works for all apps. I'd like to scope this to email-alert-frontend only.
Yes, the docker --live
command is the default way to move through this particular app and render it's pages - this wasn't clear. So as part of this PR there was a rejected update to the README
of this app to note this startup command for ease, as this is a bit of an anomaly.
This README
update request prompted further conversation about /startup.sh
and docker in general so was moved to an issue to not obstruct the PR.
I think that most of the problems highlighted in this issue could be solved by shifting the order of the documentation, with Docker listed before startup.sh; that way people would pick that up first.
Potentially, any of the docs could be updated to make this clearer (the Frontend Docs, the Docker docs, the app's README
..)
As well, the startup script documentation should have a caveat added saying that this might not work with certain apps, including email alert frontend. As we know about more apps, we can update the documentation.
I think this also applies to frontend
as well, I'm not sure if there are others.
👋🏼 @injms do you think this is worth updating as part of https://github.com/alphagov/govuk-ithc-documentation/issues/116 or should we close this issue?
What?
Viewing this app for the first time and starting up in isolation (to resolve an a11y issue) it's not clear that the typical Frontend startup process outlined in the docs does not apply to this app.
In order to startup this app and to be able to move through example pages and have them render, this app has to be started using Docker and the
--live
flag.$ govuk-docker up email-alert-frontend-app-live
Why?
Without starting the up via this method, the app from a Frontend POV is unusable and you run into many errors.
Eg you cannot use
/.startup.sh
or docker without live data.PR conversation with @kevindew around updating the README advised that it would be more desirable to get consensus with startup documentation across many apps and is most likely a wider topic. Updating docs as suggested above would be misleading and would age badly