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

Refactor charm code base #29

Merged
merged 11 commits into from
Oct 31, 2023
Merged

Refactor charm code base #29

merged 11 commits into from
Oct 31, 2023

Conversation

nsklikas
Copy link
Contributor

@nsklikas nsklikas commented Oct 23, 2023

IAM-569

Refactor charm codebase. This PR includes a lot of changes, perhaps I should break it up into smaller PRs to make reviewing easier. It include changes for:

  • Remove tls integration (i think it was broken anyway)
  • Remove logrotate logic (i think it was broken anyway)
  • Rename image resource
  • Remove a bunch of info from the state and calculate it any time it is needed
  • Refactor migration logic to mimic what we do in other charms (there was a check for an already exists error that I couldn't replicate so I removed it).
  • dns_name in the openfga databag included the HTTP scheme if traefik was related. I removed it, not sure if it was untested or intentional. If there any issues with this I will revert it to the way it was (cc @alesstimec)
  • Refactor unit tests to use pytest
  • Use latest database integration lib
  • Add type annotations
  • Clean up code

Future work:

  • We need to refactor the library to include both the code for the provider and the requirer side (currently it only has a helper class for the requirer.
  • Update the charm-relation-interfaces spec.
  • COS integration

@nsklikas nsklikas changed the title Iam 569 Refactor charm code base Oct 23, 2023
@nsklikas nsklikas requested a review from a team October 23, 2023 11:39
@nsklikas nsklikas changed the base branch from main to IAM-568 October 23, 2023 11:40
@nsklikas nsklikas force-pushed the IAM-569 branch 2 times, most recently from 06717fa to 5e946bb Compare October 24, 2023 10:14
Base automatically changed from IAM-568 to main October 24, 2023 11:48
@nsklikas nsklikas force-pushed the IAM-569 branch 2 times, most recently from 175836b to 6e6be38 Compare October 25, 2023 09:07
@nsklikas nsklikas force-pushed the IAM-569 branch 2 times, most recently from 0b21135 to 6a143b0 Compare October 25, 2023 14:15
@nsklikas nsklikas force-pushed the IAM-569 branch 2 times, most recently from 7437237 to a17b7ff Compare October 26, 2023 17:45
Copy link
Contributor

@shipperizer shipperizer left a comment

Choose a reason for hiding this comment

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

looks legit

src/constants.py Show resolved Hide resolved
@nsklikas nsklikas merged commit 0fdd1e8 into main Oct 31, 2023
3 checks passed
@nsklikas nsklikas deleted the IAM-569 branch October 31, 2023 13:09
logger.info("workload container not ready - deferring")
event.defer()
return

def _on_peer_relation_changed(self, event):
self._container.restart(SERVICE_NAME)

Choose a reason for hiding this comment

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

I would put it in a try except block, in case it fails to restart

Suggested change
self._container.restart(SERVICE_NAME)
try:
self._container.restart(SERVICE_NAME)
except ChangeError as err:
logger.error(str(err))
self.unit.status = BlockedStatus("Failed to restart the container, please consult the logs")
return

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.

3 participants