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

PR For the Charm Review #68

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added .codespellignore
Empty file.
5 changes: 5 additions & 0 deletions .copier-answers.yml
yhaliaw marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Changes here will be overwritten by Copier
_commit: c1ca71c
_src_path: gh:charmed-kubernetes/pytest-operator-template
charm_type: machine
class_name: GithubRunnerOperator
9 changes: 9 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[flake8]
Copy link
Contributor

@amandahla amandahla Jun 19, 2023

Choose a reason for hiding this comment

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

style: Shouldn't this file be removed? So flake8 will only be configured using pyproject.toml

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

max-line-length = 99
select: E,W,F,C,N
exclude:
venv
.git
build
dist
*.egg_info
57 changes: 57 additions & 0 deletions .github/ISSUE_TEMPLATE/bug_report.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
name: Bug Report
description: File a bug report
labels: ["Type: Bug", "Status: Triage"]
body:
- type: markdown
attributes:
value: >
Thanks for taking the time to fill out this bug report! Before submitting your issue, please make
sure you are using the latest version of the charm. If not, please switch to this image prior to
posting your report to make sure it's not already solved.
- type: textarea
id: bug-description
attributes:
label: Bug Description
description: >
If applicable, add screenshots to help explain the problem you are facing.
validations:
required: true
- type: textarea
id: reproduction
attributes:
label: To Reproduce
description: >
Please provide a step-by-step instruction of how to reproduce the behavior.
yhaliaw marked this conversation as resolved.
Show resolved Hide resolved
placeholder: |
1. `juju deploy ...`
2. `juju relate ...`
3. `juju status --relations`
validations:
required: true
- type: textarea
id: environment
attributes:
label: Environment
description: >
We need to know a bit more about the context in which you run the charm.
- Are you running Juju locally, on lxd, in multipass or on some other platform?
- What track and channel you deployed the charm from (i.e. `latest/edge` or similar).
- Version of any applicable components, like the juju snap, the model controller, lxd, microk8s, and/or multipass.
validations:
required: true
- type: textarea
id: logs
attributes:
label: Relevant log output
description: >
Please copy and paste any relevant log output. This will be automatically formatted into code, so no need for backticks.
Fetch the logs using `juju debug-log --replay` and `kubectl logs ...`. Additional details available in the juju docs
at https://juju.is/docs/olm/juju-logs
render: shell
validations:
required: true
- type: textarea
id: additional-context
attributes:
label: Additional context

17 changes: 17 additions & 0 deletions .github/ISSUE_TEMPLATE/enhancement_proposal.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
name: Enhancement Proposal
description: File an enhancement proposal
labels: ["Type: Enhancement", "Status: Triage"]
body:
- type: markdown
attributes:
value: >
Thanks for taking the time to fill out this enhancement proposal! Before submitting your issue, please make
sure there isn't already a prior issue concerning this. If there is, please join that discussion instead.
- type: textarea
id: enhancement-proposal
attributes:
label: Enhancement Proposal
description: >
Describe the enhancement you would like to see in as much detail as needed.
validations:
required: true
32 changes: 32 additions & 0 deletions .github/workflows/build_charm.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Build charm

on:
pull_request:
workflow_call:
workflow_dispatch:

jobs:
get-runner-image:
name: Get runner image
uses: canonical/operator-workflows/.github/workflows/get_runner_image.yaml@main
build-charm:
name: Build the charm
needs: get-runner-image
runs-on: ${{ needs.get-runner-image.outputs.runs-on }}
steps:
- uses: actions/checkout@v3
- name: Enable network in LXD
run: sudo iptables -I DOCKER-USER -j ACCEPT
- name: Add user to lxd group
run: sudo adduser runner lxd
- name: Initialize LXD
run: sudo -u runner lxd init --auto
- name: Install charmcraft
run: sudo snap install charmcraft --classic
- name: Pack charm
run: sudo -u runner charmcraft pack
- uses: actions/upload-artifact@v3
with:
name: github-runner-charm
path: github-runner_*.charm
retention-days: 5
12 changes: 12 additions & 0 deletions .github/workflows/comment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: Comment on the pull request

on:
workflow_run:
workflows: ["Tests"]
types:
- completed

jobs:
comment-on-pr:
uses: canonical/operator-workflows/.github/workflows/comment.yaml@main
secrets: inherit
61 changes: 61 additions & 0 deletions .github/workflows/end_to_end_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: End to end tests

on:
pull_request:
workflow_call:
workflow_dispatch:

jobs:
e2e-test:
name: end to end test
runs-on: [self-hosted, linux, x64, e2e-runner]
steps:
- name: Echo hello world
Copy link
Contributor

@amandahla amandahla Jun 19, 2023

Choose a reason for hiding this comment

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

Is there a reason for this echo here?
Would be possible to add a comment explaining that this echo is a connectivity test?

#70 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

run: echo "hello world"
- name: File permission for /usr/local/bin
run: ls -ld /usr/local/bin | grep drwxrwxrwx
- name: Test file permission for /usr/local/bin
run: touch /usr/local/bin/test_file
# "Install microk8s" step will test if the proxies settings are correct.
- name: Proxy set in /etc/environment
run: cat /etc/environment | grep HTTP_PROXY
Copy link
Collaborator

@arturo-seijas arturo-seijas Jun 20, 2023

Choose a reason for hiding this comment

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

What's the purpose of this cat /etc/environment | grep HTTP_PROXY ? Debugging? If so, how about of the rest of the proxy settings?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is to test whether the /etc/environment file was successful written to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

# "Update apt in python docker container" step will test docker default proxy settings due to
# pulling the python image.
- name: Proxy set in docker daemon
run: sudo cat /etc/systemd/system/docker.service.d/http-proxy.conf | grep HTTP_PROXY
# "Update apt in python docker container" step will test docker client default proxy settings.
- name: Proxy set in docker client
run: cat /home/ubuntu/.docker/config.json | grep httpProxy
- name: Install microk8s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we merge all the microk8s steps in ones?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which steps are you thinking about merging together?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

run: sudo snap install microk8s --classic
jdkandersson marked this conversation as resolved.
Show resolved Hide resolved
- name: Wait for microk8s
run: sudo microk8s status --wait-ready
- name: Deploy nginx for testing
run: sudo microk8s kubectl create deployment nginx --image=nginx
- name: Wait for nginx to be ready
run: sudo microk8s kubectl rollout status deployment/nginx --timeout=30m
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't 30m too much? Maybe 5m?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

- name: Update apt in python docker container
run: docker run python:3.10-slim apt update
yhaliaw marked this conversation as resolved.
Show resolved Hide resolved
dep-test:
# Test the dependencies in one job to avoid using too many runners at once.
name: dependency test
needs: e2e-test
runs-on: [self-hosted, linux, x64, e2e-runner]
steps:
- name: Docker version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we merge all these steps together? It will make debugging easier

run: docker version
- name: pip version
run: python3 -m pip --version
- name: npm version
run: npm --version
- name: shellcheck version
run: shellcheck --version
- name: jq version
run: jq --version
- name: yq version
run: yq --version
- name: install check-jsonschema
run: python3 -m pip install check-jsonschema
# Test program installed by pip. The directory `~/.local/bin` need to be added to PATH.
yhaliaw marked this conversation as resolved.
Show resolved Hide resolved
- name: test check-jsonschema
run: check-jsonschema --version
26 changes: 26 additions & 0 deletions .github/workflows/promote_charm.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: Promote charm

on:
workflow_dispatch:
inputs:
origin-channel:
type: choice
description: 'Origin Channel'
options:
- latest/edge
destination-channel:
type: choice
description: 'Destination Channel'
options:
- latest/stable
secrets:
CHARMHUB_TOKEN:
required: true

jobs:
promote-charm:
uses: canonical/operator-workflows/.github/workflows/promote_charm.yaml@promote-charm-base

Choose a reason for hiding this comment

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

Shouldn't we use @main ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

with:
origin-channel: ${{ github.event.inputs.origin-channel }}
destination-channel: ${{ github.event.inputs.destination-channel }}
secrets: inherit
9 changes: 9 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name: Tests

on:
pull_request:

jobs:
unit-tests:
uses: canonical/operator-workflows/.github/workflows/test.yaml@main
secrets: inherit
7 changes: 7 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.tox/
Copy link
Contributor

Choose a reason for hiding this comment

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

I miss a standard for this file on our charms. What do you think about using https://github.com/github/gitignore/blob/main/Python.gitignore as a base?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments

__pycache__/
*.pyc
placeholders/
*.charm
build/
.coverage
4 changes: 4 additions & 0 deletions .jujuignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/venv
*.py[cod]
*.charm
/.github
24 changes: 24 additions & 0 deletions .licenserc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
header:
license:
spdx-id: Apache-2.0
copyright-owner: Canonical Ltd.
content: |
Copyright [year] [owner]
See LICENSE file for licensing details.
paths:
- '**'
paths-ignore:
- '.github/**'
- '**/*.j2'
- '**/*.md'
- '**/*.txt'
- '.codespellignore'
- '.copier-answers.yml'
- '.flake8'
- '.jujuignore'
- '.gitignore'
- '.licenserc.yaml'
- 'CODEOWNERS'
- 'icon.svg'
- 'LICENSE'
comment: on-failure
1 change: 1 addition & 0 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* @canonical/is-charms
Loading