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

Review pull request for charm review #4

Draft
wants to merge 1 commit into
base: charm-review-base
Choose a base branch
from

Conversation

vjaincatalogic
Copy link
Contributor

@vjaincatalogic vjaincatalogic commented Oct 26, 2023

This PR is for the listing review discussion. Do not merge.

@bobadair bobadair marked this pull request as draft February 6, 2024 05:28
Copy link

@kelkawi-a kelkawi-a left a comment

Choose a reason for hiding this comment

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

Some missing elements needed for the review to pass:

  • Functionality:

    • I am unsure if my setup is incorrect according to the steps I followed in the README, but building the charm and deploying it resulted in the following error:
    Traceback (most recent call last):
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/lightkube/core/generic_client.py", line 188, in raise_for_status
        resp.raise_for_status()
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/httpx/_models.py", line 759, in raise_for_status
        raise HTTPStatusError(message, request=request, response=self)
    httpx.HTTPStatusError: Client error '404 Not Found' for url 'https://10.152.183.1/api/v1/namespaces/cloudcasa-system/pods/cloudcasa-0'
    For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/404
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "./src/charm.py", line 250, in <module>
        main(CloudcasaCharm)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/ops/main.py", line 456, in main
        _emit_charm_event(charm, dispatcher.event_name)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/ops/main.py", line 144, in _emit_charm_event
        event_to_emit.emit(*args, **kwargs)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/ops/framework.py", line 351, in emit
        framework._emit(event)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/ops/framework.py", line 853, in _emit
        self._reemit(event_path)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/ops/framework.py", line 943, in _reemit
        custom_handler(event)
      File "./src/charm.py", line 57, in _on_config_changed
        cloudcasa_pod = client.get(
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/lightkube/core/client.py", line 140, in get
        return self._client.request("get", res=res, name=name, namespace=namespace)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/lightkube/core/generic_client.py", line 245, in request
        return self.handle_response(method, resp, br)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/lightkube/core/generic_client.py", line 196, in handle_response
        self.raise_for_status(resp)
      File "/var/lib/juju/agents/unit-cloudcasa-0/charm/venv/lightkube/core/generic_client.py", line 190, in raise_for_status
        raise transform_exception(e)
    lightkube.core.exceptions.ApiError: pods "cloudcasa-0" not found

    It's better to avoid having your charm go into an error state for whatever reason. For example, if a required resource is not found, your charm can go into a blocked state until the required resource is created by the user.

    • Ensure that the README's instructions can guide the user to a functional installation.
  • Integration tests needed to validate the functionality of the charm. This can be in the form of a smoke test to ensure that the charm builds and installs correctly, as well as a simple test to ensure the charm is functional. Some examples can be found here:

  • CI/CD Github actions needed to automate the release of the charm to Charmhub.
    You may use the pre-defined operator-workflows to achieve this.

Choose a reason for hiding this comment

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

While not required, I recommend taking a look at the operator workflows we use for some of the charms developed at Canonical. These will help you with setting up the required CI/CD actions for releasing your charms on every merge with the main branch.

config.yaml Show resolved Hide resolved
@@ -0,0 +1,3 @@
ops >= 1.2.0

Choose a reason for hiding this comment

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

While not a requirement, I recommend considering an upgrade to the latest ops version of 2.1.0 as it has additional useful features.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Updated in PR #7.

Choose a reason for hiding this comment

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

Running this results in some complaints from the linter. Consider running tox -e fmt to format your code and address any other linting errors (e.g. incrase maximum line length to address E501).

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the weird flake8 exclusions and fixed all linter errors in PR #5.

Choose a reason for hiding this comment

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

The unit tests currently have a coverage of 27%. Is there a way we can increase the unit test coverage?

docs/dev-env-setup.md Show resolved Hide resolved
docs/test-env-setup.md Show resolved Hide resolved
docs/dev-env-setup.md Show resolved Hide resolved
actions.yaml Show resolved Hide resolved
@bobadair
Copy link
Contributor

  • I am unsure if my setup is incorrect according to the steps I followed in the README, but building the charm and deploying it resulted in the following error:

@kelkawi-a - I apologize, but the instructions provided in the dev-env-setup.md and test-env-setup.m files were incorrect. I have replaced those instructions with entirely new ones in PR #5 , and verified myself that the charm builds and deploys correctly when using the new instructions.

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.

None yet

3 participants