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

Kf 4242 contributing #288

Closed
wants to merge 16 commits into from
Closed

Kf 4242 contributing #288

wants to merge 16 commits into from

Conversation

ColmBhandal
Copy link
Contributor

Added contributing files for charms and contributing auto-update workflow for both. The text is quite generic but can be edited via contributing_inputs.yaml.

Workflow
Contributing files
Contributing inputs
Run it weekly
this was just used to test
we don't want it in general, it's a waste
Copy link
Contributor

@phoevos phoevos left a comment

Choose a reason for hiding this comment

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

Thanks @ColmBhandal, some nitpicks!

.github/workflows/contributing_update_all.yaml Outdated Show resolved Hide resolved
charms/jupyter-controller/contributing_inputs.yaml Outdated Show resolved Hide resolved
charms/jupyter-ui/contributing_inputs.yaml Outdated Show resolved Hide resolved
charms/jupyter-ui/contributing_inputs.yaml Outdated Show resolved Hide resolved
@phoevos
Copy link
Contributor

phoevos commented Aug 29, 2023

Apart from the above, I do have some questions:

  • Do we have context on these contributing.md files?
    • Is their content standardised across our repos?
    • Are the files added here already merged somewhere else?
    • What is the purpose of the auto-update and how is it intended to be used?
      • Are there instructions somewhere?

Most importantly, I want to understand whether the content of this PR is something that people have already agreed upon or rather something that's currently under design (if that's the case I might have more comments😇).

@ColmBhandal
Copy link
Contributor Author

Apart from the above, I do have some questions:

  • Do we have context on these contributing.md files?

    • Is their content standardised across our repos?

    • Are the files added here already merged somewhere else?

    • What is the purpose of the auto-update and how is it intended to be used?

      • Are there instructions somewhere?

Most importantly, I want to understand whether the content of this PR is something that people have already agreed upon or rather something that's currently under design (if that's the case I might have more comments😇).

Hey - please provide as much input as you would like.

The automation works as follows. The workflow in this repo delegates to a reusable workflow for updating all contributing files for all charms in the repo. Link to that workflow.

Then, for each charm, the workflow calls upon another reusable GitHub action which generates a fresh contributing file from a template. More info on that is available in the readme for the action.

@phoevos
Copy link
Contributor

phoevos commented Aug 30, 2023

Ok so the missing piece for me was the common template used for all updates, which can be found here.

This is the base template for every charm contributing.md. Then in each charm repo we must define a contributing_inputs.yaml file, which contains the context to render the common template. The most important part of the context is the charm_specific_text which holds the charm-specific body of the contributing.md file.

So in order to answer my own questions above:

  • contributing.md.template is meant to be shared across our charms
    • Any changes in the contributing.md structure should be directed there
  • Whenever we want to make changes to a charm contributing.md, we:
    • don't touch the contributing.md file directly
    • update contributing_inputs.yaml
    • wait for the automation to kick in (?)

Does the above description hold? If it does, then we really need to:

  • enhance the instructions provided here so that the purpose of the automation is clear and devs know how to use it
  • ensure that charm-specific contributing.md files aren't edited directly:
    • this could be as simple as adding a markdown comment in the contributing template, instructing devs to edit the contributing_inputs.yaml instead. Will a comment in the template be persisted in the rendered files?

Finally, what is the purpose of the weekly automation? I mean, I would expect the contributing.md files to be rendered every time I update the inputs, not a week later. I don't love the idea of making a change in the inputs now, merging it, and having to come back in a week or so to merge the change into contributing.md. I understand the value of the common template, but this is just too much effort.

@ColmBhandal
Copy link
Contributor Author

ColmBhandal commented Aug 30, 2023

Ok so the missing piece for me was the common template used for all updates, which can be found here.

This is the base template for every charm contributing.md. Then in each charm repo we must define a contributing_inputs.yaml file, which contains the context to render the common template. The most important part of the context is the charm_specific_text which holds the charm-specific body of the contributing.md file.

So in order to answer my own questions above:

  • contributing.md.template is meant to be shared across our charms

    • Any changes in the contributing.md structure should be directed there
  • Whenever we want to make changes to a charm contributing.md, we:

    • don't touch the contributing.md file directly
    • update contributing_inputs.yaml
    • wait for the automation to kick in (?)

Does the above description hold? If it does, then we really need to:

  • enhance the instructions provided here so that the purpose of the automation is clear and devs know how to use it

  • ensure that charm-specific contributing.md files aren't edited directly:

    • this could be as simple as adding a markdown comment in the contributing template, instructing devs to edit the contributing_inputs.yaml instead. Will a comment in the template be persisted in the rendered files?

Finally, what is the purpose of the weekly automation? I mean, I would expect the contributing.md files to be rendered every time I update the inputs, not a week later. I don't love the idea of making a change in the inputs now, merging it, and having to come back in a week or so to merge the change into contributing.md. I understand the value of the common template, but this is just too much effort.

Hey, that's close but not exactly how it works. I've added PRs that will hopefully explain this better and answer your questions:

  1. Contributing Update All README charmed-kubeflow-workflows#19.
  2. Contributing update README.md kubeflow-ci#108

Upon analysis, the design favours updating the template file but makes charm-specific updates a bit awkward. Charm specific updates require updating the contributing_inputs.yaml and then also either merging an automated PR (which will be immediate, not waiting a week), or else manually updating contributing.md also, which duplicates the work.

But the idea was that that was the price we have to pay for keeping these things in line with the common template. If we expect updates to the template to outweigh charm-specific updates, then this approach makes sense.

@phoevos
Copy link
Contributor

phoevos commented Sep 1, 2023

Thanks @ColmBhandal, we can wrap this after the linked PRs are merged.

What will happen after canonical/kubeflow-ci#107 is merged though? Will you manually update the contributing.mds here to match the updated template? Will a second PR be triggered the moment we merge this one to update the contributing.mds. Or will that happen on Sunday?

@ColmBhandal
Copy link
Contributor Author

Thanks @ColmBhandal, we can wrap this after the linked PRs are merged.

What will happen after canonical/kubeflow-ci#107 is merged though? Will you manually update the contributing.mds here to match the updated template? Will a second PR be triggered the moment we merge this one to update the contributing.mds. Or will that happen on Sunday?

I did the super-hacky use case of editing both contributing.md and contributing_inputs.yaml myself (duplicating the effort for myself), so no PRs should be generated. If I hadn't done that, I believe PRs would already have been generated by now.

Remove token - let's see if it breaks
Remove committer and author
@ColmBhandal
Copy link
Contributor Author

Closing this as it's too much effort. We have decided for now to go with the simpler option of just manually creating and maintaining contributing files.

@ColmBhandal ColmBhandal closed this Sep 5, 2023
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.

2 participants