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

Add recommendations for continuous integration, delivery and deployment #48

Merged
merged 12 commits into from
May 20, 2021

Conversation

stephengeller
Copy link
Contributor

What does this change?

This PR adds documentation on continuous integration, deployment and delivery at the Guardian.

Following @guardian/developer-experience 's sessions on establishing a vision for CI & CD, these documents list the current state of play for these areas as well as our vision for what it could look like in the future.

For more information and context, please see the Best Practices Index created from the above sessions, with links to research and proposal documents inside.

How to test

Verify that these documents align with our current practices and assess whether the proposal is appropriate for our department.

How can we measure success?

We have a clear set of documents listing our best practices for continuous integration, delivery and deployment.

@davidfurey
Copy link
Member

Thanks for contributing to the recommendations repo! There are definitely some great things in here. I think it makes sense to mark this as a draft PR for now, and not consider merging it until it provides clear recommendations for the approach developers should use now.

Also, since this is a public repo ideally the final version would not contain links to docs that are not public.

@stephengeller stephengeller marked this pull request as draft January 20, 2021 09:17
@stephengeller
Copy link
Contributor Author

Thanks @davidfurey , that's good advice. I'll take those notes onboard - should be feasible to at least make all information public, and I can capture current approaches better here too.

@stephengeller stephengeller marked this pull request as ready for review January 21, 2021 11:34
@stephengeller
Copy link
Contributor Author

@davidfurey your suggestions have been actioned and this is hopefully now ready for review, let me know if there is more needed.

@davidfurey
Copy link
Member

Thanks for the changes, it is much clearer now. What do you think about moving the future vision into a github issue or Google doc? My preference would be for the recommendations repo to be a living document of what we currently recommend rather than a place to articulate our vision. But I'd like to hear your and others views, because maybe future vision does belong here.

@stephengeller
Copy link
Contributor Author

Interesting - I think you're right, having in one of those two places could be better as I do agree that we should keep this centred around recommendations rather than visions.

I'd worry that the vision would get lost if in a Google doc though, so maybe an issue or even a discussion would be more suitable? It would mean we have what we are doing colocated with what we want to be doing in the future. Also keen to hear other people's thoughts though.

@stephengeller
Copy link
Contributor Author

Following a conversation on chat with @davidfurey , we have agreed to move the proposals side of this PR to a GitHub discussion and keep this PR focused on current recommendations. Will update this PR accordingly soon!

@stephengeller
Copy link
Contributor Author

Future vision has been moved to a discussion (https://github.com/guardian/recommendations/discussions/50) and removed from this PR.

continuous-deployment.md Outdated Show resolved Hide resolved
* Use TeamCity to run continuous integration tasks
* If possible, have a centralised script in the repository named `scripts/ci`
- This adheres to GitHub's [Scripts To Rule Them All pattern](https://github.com/github/scripts-to-rule-them-all)
* For public repositories, you can use [GitHub Actions](https://docs.github.com/en/actions) for some above tasks, but we are yet to develop a safe way of uploading artifacts to RiffRaff through it
Copy link
Member

Choose a reason for hiding this comment

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

Should we be suggesting this if there isn't a safe way to do it? Probably we need to discuss what is mean't by safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't see this comment until now! This is a good point, but I think this could be better explained by changing the phrasing a bit.

I've just pushed a change that aims to rephrase the recommendation from "there isn't a safe way to push artefacts to RiffRaff" to "we haven't developed a best practice for this yet", which is more accurate. I think, at the time, I meant "safe" as "you can't safely use the recommended node-riffraff-artifact in GitHub Actions as you'd have to publish/configure credentials somewhere", but this would be fairly safe to do if you configured AWS credentials in each repo and rotated them regularly.

I think a good (and safe)solution for this is possible (likely relying on Guardian org-wide tokens being set up and a custom GitHub Action to implement it), and I think it's sensible to still encourage people to use GitHub Actions as it is a great interface, free for public repositories, and safe to use for the tasks mentioned besides publishing artefacts to RiffRaff.

@stephengeller stephengeller force-pushed the sg/proposals/ci-cd branch 2 times, most recently from 6792e9d to 3752bb5 Compare May 20, 2021 06:55
## Platforms

* Use TeamCity to run continuous integration tasks
* If possible, have a centralised script in the repository named `scripts/ci`
Copy link
Member

Choose a reason for hiding this comment

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

minor: Could we add the qualifier that CI should have a single step, and that is to run this script:

Where possible, have CI execute a single, centralised script in the repository named scripts/ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good call. Added this. Also corrected my own error of scripts -> script in the process

Copy link
Contributor

@mxdvl mxdvl left a comment

Choose a reason for hiding this comment

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

Mostly Markdown formatting comments from me.

I noticed we didn’t mention anything about visual testing or code coverage. Is it too team-specific to make it into a recommendation?

continuous-deployment.md Show resolved Hide resolved
continuous-deployment.md Show resolved Hide resolved
- Set up scheduled deploys, continuous deployment and deployment restrictions in the RiffRaff UI
- Configure your RiffRaff deployment types inside your repository's `riff-raff.yaml` file

**See [continuous integration](continuous-integration.md)**
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, if you've added the link this will be turned into it automatically.

Suggested change
**See [continuous integration](continuous-integration.md)**
**See [continuous integration]**

## Tasks

You should have continuous integration set up in your repository running the following tasks where relevant:
* Running unit and integration tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a note of UI / visual regression testing here? The interface of our applications is often an important part of the integration process.

Co-authored-by: Akash Askoolum <[email protected]>
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