Review Charm for Listing #100
Open
+11,542
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`bootstack-charms-spec`.
1. `tox -e format` was doing lint 2. `lib` directory was not excluded.
The "lib" directory is fetching from the ops's library which does not necessary follow "mypy" rule. We should not validate it from our repository unless we have our own library.
* Add snap library.
This PR basically remove the unnecessary logic in the SnapService.configure because snap service now will not restart when it's configured.
Save these credentials to the clouds.yaml file, so they can be used by openstack-exporter.
This ignore rule conflicts with ruff formatter (originally it was planned to use `black` formatter so the rule existed, but we did not use it), making the ruff formatter will still format the code even if the ruff linter passes.
…t by default with tox. (#20) This makes the testing workflow a bit simpler: `tox` now runs all the quick tests only; formatting shouldn't be included, because that changing code and testing code probably should be separate. Merge mypy type checking into the lint environment, because that is also another type of static analysis, and is quick and can all be run together. This makes `tox -e lint` runs all the static analysis tools (no project code is run).
* Add integration with Grafana Agent - pull in the cos_agent interface library - add script to sync the charm libs - configure the relation with path and port for openstack-exporter metrics * Block if grafana agent is not related we don't need anything serving metrics if nothing is listening * Update charm config on grafana agent relation changes
* Add workflow for lint, static and unit test. * Add dummy unit test to make the workflow pass. * Update tox.ini and tests/unit/requirements.txt There are some requirements for testing the charm because now the cos_agent library need them.
* Add workflow to check updates for ./src/grafana_dashboards/* * Copy openstack-exporter.json from sunbeam-charms. * Add Makefile to the charm, and managed the charm operations with `make`. - Add Makefile - Fix small issue with the scripts/update-charm-libs.sh - Add scripts/check-dashboard-updates.sh to check for dashboard updates - Update CONTRIBUTING.md
And refactor to merge duplicate code. Several events only need to call the configure method, so the event handlers can be removed/merged. Fixes: #21
* Refactor workflows to smaller workflows. * Add workflow for releasing charm to latest/edge channel when push to `main`.
Previously this error was seen in the logs: ts=2024-04-03T23:09:00.715Z caller=exporter.go:126 level=error err="Failed to collect metric for exporter" exporter=nova error="failed to collect metric: running_vms, error: CPUInfo has unexpected type: <nil>" Causing `openstack_nova_running_vms` and a selection of other hypervisor related metrics to be missing from the output. This applies the workaround noted in openstack-exporter/openstack-exporter#268
* test: Init zaza integration - refactor file structure to make zaza work properly - Download snap file from github release assets * test: Fix error that clean the resource after download the snap * test: Fix wrong test module path * docs: Add deleted comment back * fix: Adjust Makefile clean command and add series to jammy func test bundle
* Add new script to sync dashboards With a flag to check if dashboards are updated (so `make check-dashboard-updates` still works). Also run the script to sync the dashboards. * Clarify BASE_URL 404
* test: Setup openstack environemnt for testing
* Add integration tests. * Reduce update-status-hook-interval for integration test.
* Support Juju 3.4 - Changing to use of Juju 3.4 controller for testing instead of Juju 3.1 - Add workflow concurrency to avoid running multiple workflows on a single PR. * Fix zaza bundle. Since juju 3.1+, the bundle need to be a valid bundle before applying the overlay bundle.
This lets us simplify the workflow, gives us more control over testing in the future, and also lets us pin charmcraft to v2 (this charm doesn't official support charmcraft 3).
…ck/sunbeam-charms/src/branch/main/charms/openstack-exporter-k8s/src (#88) This commit was generated by https://github.com/canonical/openstack-exporter-operator/blob/main/.github/workflows/sync-from-sunbeam.yaml Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com>
Add extra-bindings to the charm metadata to allow users to request legs in all OpenStack networks spaces. This may be necessary in case the OpenStack API endpoints to be queried are not available on the same network space as the one used by the credentials endpoint. While the charm doesn't explicitly use the bindingsi - meaning that the same result could be achieved with machine space contraints - having these specified as application bindings is more explicit. This is effectively the same solution used in the previous exporter charm, see https://git.launchpad.net/charm-prometheus-openstack-exporter/commit/?id=99692c082eadbf5195bb683e92a73428603efbc6 Closes #90
Upstream can take some time until releases a new snap and this is blocking the workflow. This change from using the golang-openstack-exporter (upstream) to use the charmed-openstack-exporter that is packaging the upstream code and release in the snapcraft store. With this change the charm automatically uses the snap from the snapstore and removes the upstream snap and/or snap as a resource. The resource can still be used as a way to test customized snap or an alternative solution for airgapped environments
Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com>
* block units if upstream snap is installed via resource * not run config-changed when upstream resource is used * added func test showing that units go into blocked state if upstream resource is added. * used snap cli to query snaps installed on functional tests Closes: #94
…ck/sunbeam-charms/src/branch/main/charms/openstack-exporter-k8s/src (#99) This commit was generated by https://github.com/canonical/openstack-exporter-operator/blob/main/.github/workflows/sync-from-sunbeam.yaml Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com>
* Pass cacert as a file path to cloud.yaml * Modify test_configure_ssl_ca to verify cacert is written to a file * Use pathlib for config paths * Remove redundant code and improve ssl_ca file verification
jneo8
added a commit
that referenced
this pull request
Sep 13, 2024
jneo8
added a commit
that referenced
this pull request
Sep 13, 2024
|
Could you please leave a note here on anything that you have addressed already? Just to make it easier for me to track what still would need to be resolved please |
* update .gitignore * update .gitignore * update .gitignore * update .gitignore * update .gitignore --------- Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com>
* refactor: Address review comments on #100 - Remove `# TODO: can we remove the base64 encoding?` on `.github/workflows/check.yaml` - Add explanation when both `openstack-exporter` and `snap_channel` are set. - Change string-formatting to f-string - Remove double negative - Raise SnapError directly - Add explanation why we need snap local installation
* update .github/workflows/promote.yaml * update .github/workflows/release.yaml --------- Co-authored-by: soleng-terraform[bot] <168111096+soleng-terraform[bot]@users.noreply.github.com> Co-authored-by: Samuel Allan <[email protected]>
Raise errors for snap install or refresh failure. Raise an error because it's a fatal failure, and juju will auto-retry the hook, which may succeed on retry if the error was transient. Improve blocked status message for if it detects snap not installed or service not active in the status hook. I think Blocked is better than Error status here: - auto retrying the status hook isn't going to fix anything - blocked status lets the charm provide a more useful status message to the user - blocked status will automatically update to active on the next update-status hook once the issue is resolved Partially fixes: #108
|
Given that there have been quite a few changes, could you update this PR with the latest code? Then I can take a look at the changes and also review the tests 🙂 |
Hi @jdkandersson , sorry for the delay. I merge the upstream/main to this review branch. |
jdkandersson
reviewed
Oct 8, 2024
| CHANNEL_COLUMN = 3 | ||
|
|
||
|
|
||
| class OpenstackExporterBaseTest(unittest.TestCase): |
There was a problem hiding this comment.
The Charm style guide asks using pytest-operator using pytest: https://juju.is/docs/sdk/styleguide#heading--functional-tests, is that applicable here?
There was a problem hiding this comment.
I think it is applicable, but this charm uses zaza to manage integration/functional tests. Switching to pytest-operator would be a large effort here, as it would involve rewriting all the tests. I'm not sure why we went with zaza rather than pytest-operator actually.
There was a problem hiding this comment.
@tonyandrewmeyer any thoughts here please?
There was a problem hiding this comment.
Charm-tech is intending to develop a new integration test tool next cycle to replace using pytest-operator/python-libjuju, strongly based on the Juju CLI (OP050, although that's very much still being developed).
I'd definitely encourage moving away from Zaza, but this would be a bad time to do that, rather than a few months from now.
There was a problem hiding this comment.
The reason for using zaza is because we need to deploy whole Charm Openstack for testing.
The effort to change that will be re-implement the configure function zaza.openstack.charm_tests.setup.setup_export_ssl_ca_config, changing our CI pipeline, and somehow re-implement our integration test. It's not a small change from my POV.
According to Tony's information. Can we open a issue to replace zaza with the new integration test tool as follow up?
| ({"cache": True, "cache_ttl": "200s"}), | ||
| ], | ||
| ) | ||
| def test_config_changed(self, config, mocker): |
There was a problem hiding this comment.
Not part of the formal review
I would consider writing documentation here for what the test is doing. You could use the arrange/act/assert framework for them
jdkandersson
approved these changes
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR for reviewing charm code as defined in: https://discourse.charmhub.io/t/reviewing-charms/11698.