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

Implement tooling best practices #7

Merged
merged 21 commits into from
Jan 5, 2024
Merged

Conversation

carlcsaposs-canonical
Copy link
Contributor

Copy link
Contributor

@MiaAltieri MiaAltieri left a comment

Choose a reason for hiding this comment

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

amazing work here Carl, your help here is greatly appreciated. Some comments / questions added. :)

Its great learning from you and your work here


build:
name: Build charms
# todo: update version
Copy link
Contributor

Choose a reason for hiding this comment

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

[already done]](

uses: canonical/data-platform-workflows/.github/workflows/build_charms_with_cache.yaml@v6
) 🎉

charmcraft.yaml Outdated
@@ -12,6 +12,7 @@ bases:

parts:
charm:
# todo: remove binary packages
Copy link
Contributor

Choose a reason for hiding this comment

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

noob question, why do these need to be removed? last I checked charm wouldn't pack without these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're supposed to build python packages from source when possible

using something like this: https://github.com/canonical/mysql-operator/blob/f757f4e5e0c0719e8577adac2851274dee8da8a1/charmcraft.yaml#L24-L29

downside is that builds take a lot longer

tox.ini Outdated

poetry install --only fmt
poetry lock --no-update
# TODO: remove black?
Copy link
Contributor

Choose a reason for hiding this comment

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

noob question, why remove black

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe ruff replaces it

Copy link

codecov bot commented Jan 4, 2024

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@carlcsaposs-canonical carlcsaposs-canonical marked this pull request as ready for review January 4, 2024 14:33
Comment on lines +44 to +46
tenacity = "^8.1.0"
pymongo = "^4.3.3"
ops = "^2.4.1"
Copy link
Contributor Author

@carlcsaposs-canonical carlcsaposs-canonical Jan 4, 2024

Choose a reason for hiding this comment

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

@MiaAltieri FYI this might cause a headache in the future—your integration tests probably shouldn't depend on ops (but it looks like they import code which imports ops)

Copy link
Contributor

@MiaAltieri MiaAltieri left a comment

Choose a reason for hiding this comment

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

Thank you so much Carl, this is great

Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

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

Great work, thank you Carl!

@MiaAltieri MiaAltieri merged commit 9150159 into 6/edge Jan 5, 2024
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.

3 participants