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

community,wg,code-quality: establish working group code-quality #333

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

dhiller
Copy link
Contributor

@dhiller dhiller commented Oct 10, 2024

What this PR does / why we need it:

Create wg charter and add sigs.yaml entry for wg-code-quality.

Update sig-list.md to reflect changes.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #327

Special notes for your reviewer:

This PR needs to get rebased after #346 got in

@kubevirt-bot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 10, 2024
@dhiller dhiller force-pushed the wg-code-quality branch 2 times, most recently from 2d2c61f to c28ffcb Compare October 10, 2024 11:02
@dhiller
Copy link
Contributor Author

dhiller commented Oct 22, 2024

Pinging folks for awareness - @0xFelix @lyarwood @iholder101 @EdDev @fossedihelm @enp0s3 @jcanocan @xpivarc @nunnatsa @ormergi

All, feel free to nominate yourself for a wg chair if you so desire by adding a suggestion with your github handle to the wg charter document.

Copy link
Member

@dankenigsberg dankenigsberg left a comment

Choose a reason for hiding this comment

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

(partial review)

wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
Copy link
Member

@EdDev EdDev 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 @dhiller for pushing this forward.
This is basically formalizing an ongoing active effort, allowing us to centralize, prioritize and learn how to make out code base cleaner/better.

wg-code-quality/wg-charter.md Show resolved Hide resolved

## Deliverables

The inter-SIG process definition will be manifested in a document that defines the process itself, also how and when it is executed.
Copy link
Member

Choose a reason for hiding this comment

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

I think the deliverables should be the result of the scope points (objectives in the current naming), more or less.

I like the general wording of the device-manager WG:

The WG will coordinate the delivery of KEPs and their implementations by the participating SIGs. Interim artifacts will include documents capturing use cases, requirements, and designs; however, all of those will eventually result in KEPs and code owned by SIGs.

We could re-word it a bit to fit better to our naming (or lack of it) and scope:

The WG will coordinate the prioritization and delivery of design-proposals, documentation, implementations and other content that aims to improve the code quality of the project.

@EdDev
Copy link
Member

EdDev commented Oct 27, 2024

Regarding the the chair and roles in general assignment, it will be useful to have a ref or and explanation (and example?) of what that means. Then it will be clearer to members what they need to do and estimate the needed investment.

I think we need a minimum of 2 chairs, preferable 3.
I am not really clear on how a nomination is done when a WG is created, it is a bit odd.
In any case, given my high interest in this WG success and my assumptions on what the role requires, please consider me as one of the chairs.

@dhiller
Copy link
Contributor Author

dhiller commented Nov 4, 2024

Thank you @dhiller for pushing this forward. This is basically formalizing an ongoing active effort, allowing us to centralize, prioritize and learn how to make out code base cleaner/better.

@EdDev thanks for the review - I've changed wording accordingly, hope that this conveys better what we want to achieve.

Also I've updated with the new links to the meeting room, and added a paragraph for disbanding. This I've left vague intentionally.

One note though: this WG IMHO is about creating the process (which is kind of a meta activity) that will serve as a vehicle for continuous improvement. I think that we can iterate on that process, the docmentation and the automation continuously, so therefore the WG might disband after a certain "good enough" set of processes and documentation is in place.

@dhiller
Copy link
Contributor Author

dhiller commented Nov 5, 2024

/hold to wait for rebase after #346 merged

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2024
@dhiller dhiller marked this pull request as ready for review November 5, 2024 11:17
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2024
Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

One note though: this WG IMHO is about creating the process (which is kind of a meta activity) that will serve as a vehicle for continuous improvement. I think that we can iterate on that process, the docmentation and the automation continuously, so therefore the WG might disband after a certain "good enough" set of processes and documentation is in place.

The term "process" is a bit to vague to me.
I would expect the WG to facilitate and manage the quality effort until enough tooling, documentation, code structure, reviewer and maintainer awareness is achieved. Once the code quality reached a reasonable state, the WG can be dismissed.

My remaining comments is all about this "process" I guess.
It is not a stopper though, we can and probably should sync on the expectation in the next WG meeting.
Per initial discussions we can keep on updating the charter to reflect what we learn and where we aim to go with this.

* Focusing on selected topics.
* Monitoring and identification of code-quality issues.

The WG will therefore work on processes through which the above goals can be achieved.
Copy link
Member

Choose a reason for hiding this comment

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

work on processes

I am unsure if this is clear enough to keep it in the text.

As I see it, the WG should work on providing the means to improve the quality until a point where it is well integrated in the projects. The points above hints what we expect to have in the end.

I do not think it is limited to creating processes, it is there to facilitate the effort to create content, refactor code, etc. As it is already mentioned in the points above, I think this sentence is redundant.

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 have changed the In Scope section by changing the last sentence to what you pointed out before, which IMHO brings it down into an actionable goal. WDYT?

Comment on lines 7 to 8
* (short-term) **focus on the improvement of code quality** and
* (long-term) **create an inter-SIG process for identifying, agreeing on and addressing code quality issues**
Copy link
Member

Choose a reason for hiding this comment

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

For me, line 14 is summarizing this and describe why the WG is needed.
The long-term goal here, which mentions a processes, is not very clear to me.
I would expect the WG to have artifacts from which the quality will be improved and sustained over time.

This may be partially achieved through processes, but I am unsure if that is the "solution" or how it will look-like.

Copy link
Contributor Author

@dhiller dhiller Nov 7, 2024

Choose a reason for hiding this comment

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

Agree - see above comment.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I do not see this inter-SIG process in my mind and therefore have hard time to understand and agree on it being a goal. I feel this section is redundant as everything is described in the next section.

However, this is a small part compared to all this charter content, so I should not get stuck on it.
We can raise it later and adjust per need.

@dhiller
Copy link
Contributor Author

dhiller commented Nov 7, 2024

One note though: this WG IMHO is about creating the process (which is kind of a meta activity) that will serve as a vehicle for continuous improvement. I think that we can iterate on that process, the docmentation and the automation continuously, so therefore the WG might disband after a certain "good enough" set of processes and documentation is in place.

The term "process" is a bit to vague to me. I would expect the WG to facilitate and manage the quality effort until enough tooling, documentation, code structure, reviewer and maintainer awareness is achieved.

That last sentence is a good one, I will add that to the charter if you don't mind.

Once the code quality reached a reasonable state, the WG can be dismissed.

I disagree with that the WG should disband after the code quality has reached a reasonable state. I think that it should disband after the above described facilitation and management is complete - meaning that tooling, docs etc. are in effect in order to not let the quality deteriorate below a certain lower boundary.

@dhiller
Copy link
Contributor Author

dhiller commented Nov 7, 2024

@EdDev I've updated the charter - maybe that is good enough for a start? What do others think?

@dhiller dhiller requested a review from EdDev November 7, 2024 08:48
sigs.yaml Show resolved Hide resolved
wg-code-quality/wg-charter.md Show resolved Hide resolved
sigs.yaml Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
Create wg charter and add sigs.yaml entry.

Signed-off-by: Daniel Hiller <[email protected]>
Signed-off-by: Daniel Hiller <[email protected]>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
@dhiller
Copy link
Contributor Author

dhiller commented Nov 14, 2024

I did a final polish, hope that's good enough for now - @orelmisan @lyarwood @EdDev @iholder101 PTAL again.

Thanks all for your input!

@aburdenthehand this should be nearing the approval phase.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
Copy link
Member

@orelmisan orelmisan 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 @dhiller!

Copy link
Contributor

@iholder101 iholder101 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @dhiller!!

/lgtm

Comment on lines 35 to 38
* All SIGS claiming code ownership and producing code, currently
* SIG Compute
* SIG Network
* SIG Storage
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: observability? testing? documentation?
which other SIGs do we currently have? :D

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense for now, based on SIGs with current charters, although SIG-observability has an open PR so it would be worth including them.
Is the intention to update this as other SIGs are formalised?

@EdDev
Copy link
Member

EdDev commented Nov 14, 2024

/hold to wait for rebase after #346 merged

It has been merged already.
/unhold

@aburdenthehand , can we merge this?

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2024
Copy link
Member

@lyarwood lyarwood left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@aburdenthehand aburdenthehand left a comment

Choose a reason for hiding this comment

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

Good stuff. I have a few suggestions and an additional comment to Itamar's.

wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
wg-code-quality/wg-charter.md Outdated Show resolved Hide resolved
Comment on lines 35 to 38
* All SIGS claiming code ownership and producing code, currently
* SIG Compute
* SIG Network
* SIG Storage
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense for now, based on SIGs with current charters, although SIG-observability has an open PR so it would be worth including them.
Is the intention to update this as other SIGs are formalised?

Co-authored-by: aburdenthehand <[email protected]>
Signed-off-by: Daniel Hiller <[email protected]>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@aburdenthehand
Copy link
Member

/approve

@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aburdenthehand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2024
@kubevirt-bot kubevirt-bot merged commit 640347a into kubevirt:main Nov 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal for new WG code-quality
8 participants