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 docs #10

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Update docs #10

merged 3 commits into from
Jun 13, 2024

Conversation

kelkawi-a
Copy link
Collaborator

This PR updates the charm's documentation and deployment instructions.

Note: Until the charm is published on Charmhub, the instructions in the README.md may not be fully functional.

Copy link

@mertalpt mertalpt left a comment

Choose a reason for hiding this comment

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

Two of the comments are more general than this PR, I don't really expect you to do anything about them.

These tests validate extensive linting and formatting rules. Before creating a
PR, please run `tox` to ensure proper formatting and linting is performed.

### Deploy

Choose a reason for hiding this comment

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

suggestion (if-minor): I would love to replace these type of instructions with the use of multipass and the charm-dev image.

On the other hand I guess these can be taken to be a reference than a tutorial. I can link the Airbyte developer guide doc for the steps if you want the multipass route.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could mention the use of multipass as an option, but I'm not sure how that would work when it comes to working with ingress.

juju debug-log
```

#### Deploy Charm

Choose a reason for hiding this comment

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

thought: I wonder if we could put a Terraform plan into the repository to automate this section of local deployments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Including a Terraform plan is a great idea, but I don't think it should replace this section as it helps figure out if something went wrong at each step.

CONTRIBUTING.md Show resolved Hide resolved
This charm is used to deploy Airbyte server in a k8s cluster. For a local
deployment, follow the following steps:

#### Install Microk8s
Copy link

Choose a reason for hiding this comment

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

Another idea for docs, I'm still hazy on what the recommended approach is for charm docs. Some charms like observability and identity team have their docs on charmhub. But the company read-the-docs is really nice as it lets you write docs in a dedicated repo with better syntax and formatting support, better CI/CD dedicated to docs, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The docs here will eventually follow a similar path to the Temporal charm docs and live on Charmhub.

Copy link

Choose a reason for hiding this comment

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

I wonder if the deploy section shouldn't be moved into a separate docs folder, it reads like a how-to / tutorial. Fine for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above, these will eventually be refactored to the tune of the diataxis framework.

Copy link

@AmberCharitos AmberCharitos left a comment

Choose a reason for hiding this comment

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

LGTM, some suggestions on additional detail that could be included.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kelkawi-a kelkawi-a merged commit 80703c7 into main Jun 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants