Refactor charmcraft.yaml files to use python plugin and st124 notation

[NohaIhab]

Closes #646

TODO before merging to main: pin the quality checks workflow back to main once canonical/charmed-kubeflow-workflows#91 gets merged.

Note to reviewer: the bundle integration tests are failing due to charmcraft 3.x/edge not working correctly with parallel builds, this will be resolved once #645 is merged, where charmcraftcache is being used so the issue is no longer present.

PR is pointing to the dev brach KF-6684-refactor-ci-with-build, so it is expected to be merged in red and the CI will be addressed in:
#651
#654
#650

[orfeas-k]

As a general comment, all those comments result in a not so readable charmcraft.yaml file, specially when reviewing changes. Could we instead document those in an issue e.g. Data & AI charmcraft.yaml conventions or whatever could be more appropriate and just link to that issue at the top? cc @carlcsaposs-canonical

[carlcsaposs-canonical]

As a general comment, all those comments result in a not so readable charmcraft.yaml file

imo, the comments improve readability since they explain the behavior of charmcraft, why certain things are included in the charmcraft.yaml, and what the impact will be of editing charmcraft.yaml

the comments are most relevant for someone modifying charmcraft.yaml, and future modifications made to the charmcraft.yaml will hopefully include similar comments

Could we instead document those in an issue e.g. Data & AI charmcraft.yaml conventions or whatever could be more appropriate and just link to that issue at the top?

imo this would be analgous to removing comments from code, putting them into an issue, and linking to that issue in the code. to me, that seems less effective—I don't think people editing the charmcraft.yaml file will open that issue before making edits, so the value of these comments would be lost

[orfeas-k]

imo this would be analgous to removing comments from code, putting them into an issue, and linking to that issue in the code.

Actually this is something we do sometimes, specially if this is something affecting multiple repositories and places instead of copy-pasting the same comment to different places.

to me, that seems less effective— I don't think people editing the charmcraft.yaml file will open that issue before making edits, so the value of these comments would be lost

I think this highly depends on the content of the comment e.g. the following should prompt developers to consult the issue.

# Before making any edits to this file, make sure to consult <link to issue>

[NohaIhab]

imo this would be analgous to removing comments from code, putting them into an issue, and linking to that issue in the code. to me, that seems less effective—I don't think people editing the charmcraft.yaml file will open that issue before making edits, so the value of these comments would be lost.

I agree with @orfeas-k that the wording of the comment can be very clear so that developers check the issue, and reviewers would make sure it complies with the issue. Although I see @carlcsaposs-canonical's point on the readability.

I see value in having a centralized place rather than duplicated comments everywhere as that makes changes more simple IMO. I'm interested in how is it that you handle changes to the charmcraft.yaml? Do you have an automated way of making the changes across all repos? @carlcsaposs-canonical

[carlcsaposs-canonical]

Do you have an automated way of making the changes across all repos?

it's semi-automated

I'd suggest keeping the comments for a few reasons:

• individual charms will probably make charm-specific modifications that imo should be commented in a similar manner (e.g. build-packages needed for specific python deps)—don't think that'd work well with a single issue
• consistency with the data repos

also:

• I don't think most engineers on the data team will bother opening an issue before editing, so that's why we have them inline
• people reviewing diffs in charmcraft.yaml (in PRs) also probably won't open the issue, or might not even see the issue in the diff)
• imo, the comments are easier to read inline, since they apply to specific parts of the charmcraft.yaml

[NohaIhab]

re #648 (comment)

@carlcsaposs-canonical you raise some good points and I don't have a strong argument against it. What do you think @orfeas-k ?

[orfeas-k]

individual charms will probably make charm-specific modifications that imo should be commented in a similar manner

I think that having an exception to a rule - if needed - doesn't mean that the rule should be discarded ie we can have an issue speccing out charmcraft.yaml's file structure but still keep individual charm divergences inline.

consistency with the data repos

I don't think we should enforce consistency for the sake of consistency, rather for its benefits.

might not even see the issue in the diff

This is a valid argument given that reviews take place only in github's UI, usually without opening the whole file.

I think the rest of the points are personal takes on the matter. To me, the centralized issue approach offers:

• more readability: I can read fast the actual configuration rather than explanations of it. For explanations, I can head to the linked issue.
• more trackable documentation of decisions taken, and also previous ones (which may be lost once the code changes).

which are two strong benefits. I 'm not aware though how this can affect the semi-automated way of updating that @carlcsaposs-canonical are responsible for.
That being said, I don't find this as blocker, so I 'm leaving an approval and you can proceed with merge @NohaIhab. Let's discuss though with the whole team during MLOps office hours before propagating to all of our repos. @carlcsaposs-canonical you 're more than welcome to join if you 'd like.

[carlcsaposs-canonical]

consistency for the sake of consistency, rather for its benefits

the benefits are: easier to keep in sync with data repos when changes are made & easier for people to collaborate/debug across data & ai charms