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

CI/CD deployment docs #2389

Merged
merged 5 commits into from
Jan 8, 2025
Merged

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Nov 28, 2024

Talks about how users can test their apps in the CI and about packaging their apps in Docker images for CD.

Left to do:

Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
Signed-off-by: Mihovil Ilakovac <[email protected]>
@infomiho infomiho marked this pull request as ready for review November 29, 2024 07:59
**Continuous Deployment (CD)** refers to the automatic deployment of code changes to the production environment. This is commonly know as "push to deploy" and frees developers from having to manually deploy code changes.

## Running tests in CI
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be under deployment, since CI is a separate thing from deployment.

Maybe it would be better if we had a part of docs called Testing and then tehre we can have CI part?

Copy link
Contributor Author

@infomiho infomiho Jan 3, 2025

Choose a reason for hiding this comment

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

Can we do that when we have all the deployment docs merged? I don't want to spawn a new Testing docs effort. Let's keep this here for now and then we can rework the Testing docs in the future and move this bit there.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will leave that to you to figure out! I thought maybe creating this new page in docs is clear cut, but if that is too much for now, I get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue: #2439

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Looking good! This one feels a bi tless claer to me, regarding what should be here. Seems every easy to me to go into too much details, or to not give enough info. Since it is more CI/hosting provider specific than Wasp specific. So I think it is ok to keep this as light as we can.

web/docs/deployment/ci-cd.md Show resolved Hide resolved
We'll show you how to run end-to-end tests in CI using the [Github Actions](https://github.com/features/actions) as our CI and the [Playwright](https://playwright.dev/) as our e2e testing framework.

1. Check our example app and its e2e tests in the [e2e-tests](https://github.com/wasp-lang/e2e-test-example/tree/main/e2e-tests) directory.
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm should this point to release branch? Usually in docs we aim to point at release branch, since we know it is in sycn with the released version of the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm linking here to a separate repository that only has a main branch.

Copy link
Member

Choose a reason for hiding this comment

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

All right I missed that! Hos iw that, don't we have example app with e2e tests in our wasp repo? How is it that we also have this external one? Should we be linking to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have e2e tests running in our wasp repo but it's quite complex IMHO so I wanted to offer a simple example repo with the e2e tests so people can just grab them and use them. Maybe in the future when we work on #2024 we can link to the consolidated app.

Comment on lines 33 to 52
import { expect, test } from "@playwright/test";
import { generateRandomUser, logUserIn } from "./utils";

const user = generateRandomUser();

test.describe("basic user flow test", () => {
test("log in and add task", async ({ page }) => {
await logUserIn({ page, user });
await expect(page).toHaveURL("/");
await expect(page.locator("body")).toContainText("No tasks yet.");

// Add a task
await page.fill('input[name="description"]', "First task");
await page.click('input:has-text("Create task")');
await expect(page.locator("body")).toContainText("First task");
});
});
```
</details>
Copy link
Member

Choose a reason for hiding this comment

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

You can probably drop this? It is just a small snippet, and it risks becoming out of date, we have to maintain it, real code is anyway in that example app you linked to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to include this code example here to motivate people to write these tests. The example app is nice, but this is available while going through the docs and maybe better understand what e2e tests mean without leaving the docs.

I get your concern about the code being outdated - but the example app code will get outdated over time as well :)

Copy link
Member

Choose a reason for hiding this comment

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

My feeling is that maintanability trumps the other arguments here. We will keep the example app up to date though! At least we should, I would expect that.

Comment on lines 75 to 78
- for our server app
- for our client app
4. Notify your deployment provider to deploy the new image.
Copy link
Member

Choose a reason for hiding this comment

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

This can depend on their hostirng provider, right? If I am correct some of them have their own registries so this looks more like just a single command, or maybe you just give them a branch with Dockerfile, stuff like that?
Also client app doesn't have to be in Dockerfile.
Also might be worth mentiong here that we provide Dockerfile for the server.

And now that I am writnig all this, sounds to me like this is quite complex and we should instead point them to the part of deployment docs that describes it in details? Maybe just be very simple here, try not to assume much, link to the part with dpeloying Wasp app, and say that they will want to use whatever method their hosting providers reocmmends for CD. And an example deployment of ours, that is cool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rewritten this line as For some providers: notify them to deploy the new app version.

Also client app doesn't have to be in Dockerfile.

I've mentioned an alternative way to deploy the client 👍

sounds to me like this is quite complex

Yep, it's quite complex, that's why I wanted to explain the concept from a higher level a bit. We our self-hosted deployment docs that talk a lot about using CD.

@infomiho infomiho mentioned this pull request Jan 3, 2025
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

Approved, but check a couple of new comments.

@infomiho infomiho merged commit d7c7adb into miho-deployment-docs Jan 8, 2025
@infomiho infomiho deleted the miho-deployment-docs-ci-cd branch January 8, 2025 12:16
infomiho added a commit that referenced this pull request Jan 8, 2025
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